Closed
Bug 732546
Opened 12 years ago
Closed 12 years ago
IonMonkey: StackIter should iterate over Inlined frames.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
21.18 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
StackIter is able to wallk IonFrames, but not inlined frames. This is needed to provide good stack information in some test which are introspecting the stack and also to provide a JS stack under gdb (Bug 729833)
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #603370 -
Flags: review?(dvander)
Comment on attachment 603370 [details] [diff] [review] Support iteration over inlined Ion frames Review of attachment 603370 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonFrameIterator.h @@ +70,5 @@ > + // We cannot declare a BwdInlineFrameIterator here due to multiple circular > + // dependencies, so we need to keep the IonFrameIterator to build a new one > + // each time and save the pc & script. > + const IonFrameIterator *bottom_; > + size_t nbFrames_; for counting, we usually use "nframes" or "frameCount" ::: js/src/ion/IonFrames.cpp @@ +55,5 @@ > +namespace ion { > + > +// Recover some part of the stack to read the snapshots and iterate over inlined > +// frames. > +class BwdInlineFrameIterator It took me a minute to realize what "Bwd" meant. This should be expanded to "Backward" (assuming that's what it is) or maybe rename the class to "InlineFrameReverseIterator" to match other iterator classes. @@ +72,5 @@ > + inline jsbytecode *pc() const { > + return pc_; > + } > + > + BwdInlineFrameIterator &operator--(); Not a big deal, but typically reverse iterators use ++. @@ +245,5 @@ > +InlineFrameIterator::getInlinedFrame(size_t nb) > +{ > + size_t nbFrames = 0; > + BwdInlineFrameIterator bit(*bottom_); > + while(nbFrames != nb && bit.more()) { nit: space in between while and ( ::: js/src/ion/IonFrames.h @@ +407,5 @@ > { > + if (type_ == IonFrame_Exit && current()->prevType() == IonFrame_Entry) > + return false; > + if (type_ != IonFrame_Entry) > + return true; I think this would read a little better if the second test and final return were inverted. @@ +423,5 @@ > +IonFrameIterator::hasCalleeToken() const > +{ > + // Inclusive list of frames having a calleeToken. > + return type_ == IonFrame_JS || > + type_ == IonFrame_Entry || The callee token is part of the callee, and an entry frame is never a callee. So it shouldn't be included in this test. For example, the Entry frame returning true for "hasScript" is weird - in fact we probably shouldn't query anything about the entry frame.
Attachment #603370 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 3•12 years ago
|
||
The logic is the same, but a few things changed in the middle, like renaming functions, moving some to IonFrames-inl.h, etc … And this also fix the problem encountered with DumpStack (Bug 729833) because it was iterating on frame with no legitimate calleeToken (entry frame).
Attachment #603370 -
Attachment is obsolete: true
Attachment #603892 -
Flags: review?(dvander)
Updated•12 years ago
|
Attachment #603892 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/62fdf1a5235d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•