Closed
Bug 706692
Opened 13 years ago
Closed 13 years ago
IonMonkey: Add support for StackIter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(3 files, 1 obsolete file)
393 bytes,
application/javascript
|
Details | |
5.70 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
11.76 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•13 years ago
|
||
Attachment #578172 -
Flags: review?(luke)
Assignee | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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•13 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.
Updated•13 years ago
|
Attachment #578172 -
Flags: review?(luke) → review+
Comment 6•13 years ago
|
||
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•13 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•13 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•13 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•13 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
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•