Added VM support for isolate stacktrace status.

Added VM support for isolate stacktrace status.
Committed: https://code.google.com/p/dart/source/detail?r=20898

[Tom Ball, 2013-03-28 20:33:27]

Stack trace support now implemented in isolate.cc (no exception.cc changes),
returning the following JSON format:

{ "handle": isolate handle,
  "stacktrace": [
    { "url": script URL,
      "line": line number,
      "function:" unqualified function name,
      "local_vars": [
        { "name": variable name,
          "pos": token start position,
          "end_pos": token end position,
          "value": value.ToCString()
        }, ...
      ]
    }, ...
  ]
}

It's easy to change this data if something is missing or not needed.

[siva, 2013-03-28 22:09:00]

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:711: String::Handle(frame->function().UserVisibleName());
These handle creations can be hoisted outside the for loop that would ensure we
reuse handles and not create new ones each time.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:714: buffer.Printf("\"function\": \"%s\", ",
function.ToCString());
We probably also need the code object associated with the frame in case we want
to be able to click and get to the generated code for the function (this frame
could be running optimized code or unoptimized code).

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:716: intptr_t n_local_vars = frame->NumLocalVariables();
Do we need a detailed local variable list for each frame to be collected here.
It could be another call when somebody clicks on the stack frame link....

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:722: Instance& value = Instance::Handle();
These handle creations can be hoisted outside the for loop that would ensure we
reuse handles and not create new ones each time.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:733: isolate->stacktrace_ = strndup(buffer.buf(),
buffer.length());
Who is responsible for freeing stacktrace_ ?

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:742: status_sync = new Monitor();
Why do we allocate a new monitor for each stack trace request?

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:743: ScheduleInterrupts(Isolate::kApiInterrupt);
Not sure if kApiInterrupt bit is the right one to use here.
Why could send an OOB message by posting the request string as the OOB message
or grab a new interrupt bit for VM stats and have it process the request string.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:771: if (!strncmp(p, trace_request,
strlen(trace_request))) {
It might be easier for adding new commands if we made this a table driven
dispatch...

[Tom Ball, 2013-03-29 22:39:31]

PTAL.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:711: String::Handle(frame->function().UserVisibleName());
On 2013/03/28 22:09:00, siva wrote:
> These handle creations can be hoisted outside the for loop that would ensure
we
> reuse handles and not create new ones each time.

Good idea -- done.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:714: buffer.Printf("\"function\": \"%s\", ",
function.ToCString());
On 2013/03/28 22:09:00, siva wrote:
> We probably also need the code object associated with the frame in case we
want
> to be able to click and get to the generated code for the function (this frame
> could be running optimized code or unoptimized code).

I added a "code": { "alive": bool, "optimized": bool } section. As we discussed
further code details would be returned in a separate, per-frame details query.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:716: intptr_t n_local_vars = frame->NumLocalVariables();
On 2013/03/28 22:09:00, siva wrote:
> Do we need a detailed local variable list for each frame to be collected here.
> It could be another call when somebody clicks on the stack frame link....

As we discussed, I removed this section in favor of a separate per-frame details
query.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:722: Instance& value = Instance::Handle();
On 2013/03/28 22:09:00, siva wrote:
> These handle creations can be hoisted outside the for loop that would ensure
we
> reuse handles and not create new ones each time.

Done.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:733: isolate->stacktrace_ = strndup(buffer.buf(),
buffer.length());
On 2013/03/28 22:09:00, siva wrote:
> Who is responsible for freeing stacktrace_ ?

VmStats::WebService(), after the returned status is copied to the HTTP response.
 I added a comment to make that clear.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:742: status_sync = new Monitor();
On 2013/03/28 22:09:00, siva wrote:
> Why do we allocate a new monitor for each stack trace request?

We don't -- I was being overly conservative. Now the monitor is lazily created
once.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:743: ScheduleInterrupts(Isolate::kApiInterrupt);
On 2013/03/28 22:09:00, siva wrote:
> Not sure if kApiInterrupt bit is the right one to use here.
> Why could send an OOB message by posting the request string as the OOB message
> or grab a new interrupt bit for VM stats and have it process the request
string.

I don't know, but copied what the debugger does -- perhaps both places need
improvement, so I welcome any specific suggestions.

https://codereview.chromium.org/12842015/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:771: if (!strncmp(p, trace_request,
strlen(trace_request))) {
On 2013/03/28 22:09:00, siva wrote:
> It might be easier for adding new commands if we made this a table driven
> dispatch...

It's not quite a table, but GetStatus() now just parses the request and
dispatches to status functions.

[siva, 2013-04-01 22:07:55]

LGTM with couple of comments.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:696: static Monitor* status_sync;
= NULL;

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:726: isolate->stacktrace_ = strndup(buffer.buf(),
buffer.length());
I guess with a little tweak to the implementation of TextBuffer it would be
possible to avoid this copy, I guess we can do that in a different CL.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:785: ml.Wait();
I think you need to test a condition in a while loop before
going into wait. It is possible that the other thread might have already done a
notify before you went into wait.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:848: Isolate* isolate = reinterpret_cast<Isolate*>(addr);
Is there a way to assert that this addr is a valid isolate?

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:477: char*
DoStacktraceInterrupt(Dart_IsolateInterruptCallback cb);
Normally we try to list all the functions first and then list the fields, here
we seem to have a mix.

[Tom Ball, 2013-04-02 16:49:15]

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:696: static Monitor* status_sync;
On 2013/04/01 22:07:55, siva wrote:
> = NULL;

Done.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:726: isolate->stacktrace_ = strndup(buffer.buf(),
buffer.length());
On 2013/04/01 22:07:55, siva wrote:
> I guess with a little tweak to the implementation of TextBuffer it would be
> possible to avoid this copy, I guess we can do that in a different CL.

Agreed.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:785: ml.Wait();
On 2013/04/01 22:07:55, siva wrote:
> I think you need to test a condition in a while loop before
> going into wait. It is possible that the other thread might have already done
a
> notify before you went into wait.

Added a test that stacktrace_ is still NULL before waiting.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:848: Isolate* isolate = reinterpret_cast<Isolate*>(addr);
On 2013/04/01 22:07:55, siva wrote:
> Is there a way to assert that this addr is a valid isolate?

File issue 9600, and added TODO.

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/12842015/diff/22001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:477: char*
DoStacktraceInterrupt(Dart_IsolateInterruptCallback cb);
On 2013/04/01 22:07:55, siva wrote:
> Normally we try to list all the functions first and then list the fields, here
> we seem to have a mix.

Moved functions up with the others.

[Tom Ball, 2013-04-03 22:03:11]

Committed patchset #10 manually as r20898 (presubmit successful).