Closed Bug 732546 Opened 10 years ago Closed 10 years ago

IonMonkey: StackIter should iterate over Inlined frames.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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: 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+
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)
Attachment #603892 - Flags: review?(dvander) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/62fdf1a5235d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.