IonMonkey: Add support for StackIter

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 706699
(Assignee)

Comment 1

6 years ago
Created attachment 578172 [details] [diff] [review]
part 1: mark ion frames, fix regs repointing
Attachment #578172 - Flags: review?(luke)
(Assignee)

Comment 2

6 years ago
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.
Attachment #578173 - Flags: review?(sstangl)
(Assignee)

Comment 3

6 years ago
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.
Attachment #578174 - Flags: review?(luke)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 701888

Updated

6 years ago
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 ;)
(Assignee)

Comment 7

6 years ago
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

6 years ago
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+
(Assignee)

Comment 9

6 years ago
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)
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/a1169b0f3928
http://hg.mozilla.org/projects/ionmonkey/rev/7e0a455e2030

pushed w/ comment #8 fixes
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.