732546 - IonMonkey: StackIter should iterate over Inlined frames.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp] {away}]

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)

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Attachment: Support iteration over inlined Ion frames

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

Attachment: Support iteration over inlined Ion frames [v2]

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).

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

https://hg.mozilla.org/projects/ionmonkey/rev/62fdf1a5235d