Last Comment Bug 706692 - IonMonkey: Add support for StackIter
: IonMonkey: Add support for StackIter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
:
Mentors:
: 701888 (view as bug list)
Depends on: 706699
Blocks: 677337
  Show dependency treegraph
 
Reported: 2011-11-30 15:22 PST by David Anderson [:dvander]
Modified: 2011-12-01 17:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (393 bytes, application/javascript)
2011-11-30 15:22 PST, David Anderson [:dvander]
no flags Details
part 1: mark ion frames, fix regs repointing (5.70 KB, patch)
2011-11-30 21:19 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 2: move FrameRecovery, MachineState out of IonFrames.h (11.43 KB, patch)
2011-11-30 21:20 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 3: skeletal Ion frame support (11.76 KB, patch)
2011-11-30 21:23 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-11-30 15:22:38 PST
Created attachment 578110 [details]
test case

The attached test case was originally from bug 686927. The problem is that the VM can try to inspect the stack state and get very confused because of running Ion code. We need to make StackIter understand Ion frames.
Comment 1 David Anderson [:dvander] 2011-11-30 21:19:28 PST
Created attachment 578172 [details] [diff] [review]
part 1: mark ion frames, fix regs repointing
Comment 2 David Anderson [:dvander] 2011-11-30 21:20:17 PST
Created attachment 578173 [details] [diff] [review]
part 2: move FrameRecovery, MachineState out of IonFrames.h

The separation here isn't great, but IonFrames.h is about to be included in vm/Stack.h so I don't want it to bring in a whole bunch of Ion stuff.
Comment 3 David Anderson [:dvander] 2011-11-30 21:23:39 PST
Created attachment 578174 [details] [diff] [review]
part 3: skeletal Ion frame support

This patch allows StackIter to step over Ion frames, though currently there is no interface to ask questions about these frames. Effectively it just skips over the initial, mostly bogus js::StackFrame.

This patch also moves IonActivation (which is sort of like JaegerMonkey's VMFrame). It was in IonCompartment but it's much easier to deal with in ThreadData.
Comment 4 David Anderson [:dvander] 2011-12-01 11:31:20 PST
It looks like these patches fix all of the StackIter-related orange on TBPL, so I'll do follow-up StackIter integration in future bugs.
Comment 5 David Anderson [:dvander] 2011-12-01 13:32:10 PST
*** Bug 701888 has been marked as a duplicate of this bug. ***
Comment 6 Nicolas B. Pierron [:nbp] 2011-12-01 15:42:55 PST
Comment on attachment 578173 [details] [diff] [review]
part 2: move FrameRecovery, MachineState out of IonFrames.h

Review of attachment 578173 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure Bailouts.h/cpp is a good name for hosting the FrameRecovery class.  Otherwise the patch looks good to me.  Later, you may also want to replace the IonBailoutIterator by the SnapshotIterator ;)
Comment 7 David Anderson [:dvander] 2011-12-01 15:51:47 PST
Yeah, ideally we wouldn't have to include IonFrameIterator in vm/Stack.h. Maybe there should be a FrameRecovery.h or something. Iterating the frame contents wasn't necessary in this bug, but will need to happen in bug 686927.
Comment 8 Luke Wagner [:luke] 2011-12-01 15:54:50 PST
Comment on attachment 578174 [details] [diff] [review]
part 3: skeletal Ion frame support

Great; like the iterators.

To avoid undue IM dependency, do you think the ion headers can be organized so that ion/IonFrames.h can include perhaps just the (implementation-less) iterator declarations?  Perhaps ion/IonFrames.h would want to have, well, IonFrame information so instead you could make a new ion/FrameIterator.h whose sole goal was to not include much?

>+    void prev();

It was a bit weird for me to realize this is a mutating operation.  Perhaps advance() or operator++?

>+    ion::IonActivationIterator ion_;
>+    ion::IonFrameIterator ionFrames_;

For symmetry, can you s/ion_/ionActivations_/ ?
Comment 9 David Anderson [:dvander] 2011-12-01 16:02:14 PST
Comment on attachment 578173 [details] [diff] [review]
part 2: move FrameRecovery, MachineState out of IonFrames.h

Okay, good call, I'll ditch the middle patch then in favor of a lighter include.

Note You need to log in before you can comment on or make changes to this bug.