Allow StackFrameIterator to be used on a different thread than the isolate whose stack frames are b…

Allow StackFrameIterator to be used on a different thread than the isolate whose stack frames are being iterated over.

This is necessary to be able to use this in the profiler.

BUG=
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37886

[siva, 2014-06-30 19:49:18]

I have a meta level concern about allowing iteration over stack frames of an
isolate from a different thread.
All the code in the VM currently assumes that there will be
no concurrent access to data structures and heap, with this CL we open up the
potential for this happening.

In the profiler case under windows where this is used we
know that the isolate is suspended and hence a concurrent
access is not possible but we need some strong assertions to
ensure this doesn't get used elsewhere.

I am also wondering if it is worth investigating the possibility of running the
stack iteration code on the isolate itself (e.g different thread suspends the
isolate, sets PC to a stub that saves the current state and executes the stack
iterator and returns to where it was prior to being suspended)

[Cutch, 2014-06-30 20:31:20]

PTAL

[siva, 2014-06-30 23:26:44]

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:295: // suspender.
This comment I feel should be more detailed explaining the use
case where the StackFrameIterator object can be constructed by
a thread whose current isolate is not the passed in isolate parameter.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:297: #endif
The #ifdef looks weird here, maybe you could code the assert as

ASSERT((isolate_ == Isolate::Current()) ||
OS::AllowStackIterationFromAnotherThread());

which returns true in os_win.cc

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h
File runtime/vm/stack_frame.h (right):

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:175: // first EntryFrame.
We should document in this comment here and in DartFrameIterator clearly that if
an isolate other than the current isolate is used it is the responsibility of
the caller to ensure that the isolate is not running concurrently while this
thread is iterating over the stack frames.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:177: Isolate* isolate = Isolate::Current());
Not explicit anymore? one more param has been added.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:198: : fp_(0), sp_(0), pc_(0), stack_frame_(isolate),
isolate_(isolate) { }
why is this constructor public now?

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:258: : frames_(last_fp,
StackFrameIterator::kDontValidateFrames, isolate) { }
explicit on the other constructor?

[Cutch, 2014-07-01 14:26:09]

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:295: // suspender.
On 2014/06/30 23:26:44, siva wrote:
> This comment I feel should be more detailed explaining the use
> case where the StackFrameIterator object can be constructed by
> a thread whose current isolate is not the passed in isolate parameter.

Done.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.c...
runtime/vm/stack_frame.cc:297: #endif
On 2014/06/30 23:26:44, siva wrote:
> The #ifdef looks weird here, maybe you could code the assert as
> 
> ASSERT((isolate_ == Isolate::Current()) ||
> OS::AllowStackIterationFromAnotherThread());
> 
> which returns true in os_win.cc

Done.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h
File runtime/vm/stack_frame.h (right):

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:175: // first EntryFrame.
On 2014/06/30 23:26:44, siva wrote:
> We should document in this comment here and in DartFrameIterator clearly that
if
> an isolate other than the current isolate is used it is the responsibility of
> the caller to ensure that the isolate is not running concurrently while this
> thread is iterating over the stack frames.

Done.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:177: Isolate* isolate = Isolate::Current());
On 2014/06/30 23:26:44, siva wrote:
> Not explicit anymore? one more param has been added.

Done.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:198: : fp_(0), sp_(0), pc_(0), stack_frame_(isolate),
isolate_(isolate) { }
On 2014/06/30 23:26:44, siva wrote:
> why is this constructor public now?

Done.

https://codereview.chromium.org/354063004/diff/20001/runtime/vm/stack_frame.h...
runtime/vm/stack_frame.h:258: : frames_(last_fp,
StackFrameIterator::kDontValidateFrames, isolate) { }
On 2014/06/30 23:26:44, siva wrote:
> explicit on the other constructor?

Done.

[siva, 2014-07-01 15:44:22]

lgtm

[Cutch, 2014-07-01 17:38:22]

Committed patchset #5 manually as r37886 (presubmit successful).