Closed Bug 706692 Opened 13 years ago Closed 13 years ago

IonMonkey: Add support for StackIter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.
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.
Attachment #578173 - Flags: review?(sstangl)
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.
Attachment #578174 - Flags: review?(luke)
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.
Attachment #578172 - Flags: review?(luke) → review+
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 ;)
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 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_/ ?
Attachment #578174 - Flags: review?(luke) → review+
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.
Attachment #578173 - Attachment is obsolete: true
Attachment #578173 - Flags: review?(sstangl)
You need to log in before you can comment on or make changes to this bug.