IonMonkey: Handle f.arguments: Assertion failure: isFunctionFrame(), at ./vm/Stack.h:807




JavaScript Engine
5 years ago
5 years ago


(Reporter: nbp, Assigned: nbp)


Other Branch

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
This assertion can be reproduce with js --ion-eager ./jit-test/tests/basic/bug632901.js.  

The relevant backtrace is:

#0  0x00007ffff7bd046e in raise ()
#1  0x0000000000425767 in js::StackFrame::callee (this=0x7ffff0c910b0)
#2  0x000000000062a9d3 in js::ArgumentsObject::createUnexpected (cx=0xd7ce10, fp=0x7ffff0c910b0)
#3  0x00000000004b1b2e in fun_getProperty (cx=0xd7ce10, obj=0x7ffff0a10940, id=..., vp=0x7fffffff7d80)

Comment 1

5 years ago
Created attachment 620937 [details] [diff] [review]
Create unexpected argument object from StackIter.

This patch add the minimal support the reported bug.  It provides a similar abstraction to iterate over the arguments of the inlined ion frames.

This patch assert when the number of actual arguments is higher than the number of formal arguments, because this is not handled yet by IonMonkey frames.  Also it lack support of unreadable arguments which may cause a SEGV when we try to access the register referenced by the snapshot.  This deserve another test case and patch following this one, as a second part of this bug.
Attachment #620937 - Flags: review?(dvander)
Attachment #620937 - Flags: feedback?(luke)

Comment 2

5 years ago
Comment on attachment 620937 [details] [diff] [review]
Create unexpected argument object from StackIter.

Attachment #620937 - Flags: feedback?(luke) → feedback+
Comment on attachment 620937 [details] [diff] [review]
Create unexpected argument object from StackIter.

Review of attachment 620937 [details] [diff] [review]:

Nice work. I'm hoping the edge case doesn't bite us. Hitting it would need to involve an Ion-compiled function |f| that calls a getter or setter that tries to access |f.arguments|. That seems pretty unlikely. Worst case, if this becomes behavior we can't break - we can make sure arguments are stored at getter/setter safepoints.

::: js/src/ion/IonFrameIterator-inl.h
@@ +65,5 @@
> +        s.skip();
> +    while (count--) {
> +        // :TODO: We are not always able to read values from the snapshots,
> +        // some values such as non-gc things may still be live in registers
> +        // and cause a segfault while reading the machine state.

We can't segfault here - instead, populate with UndefinedValue (and maybe an assert so we can catch this easily when debugging).

::: js/src/vm/Stack.cpp
@@ +1467,5 @@
> +      case SCRIPTED:
> +        JS_ASSERT(isFunctionFrame());
> +        return fp()->numActualArgs();
> +      case ION:
> +        // :TODO: We need to handle actual arguments in IonMonkey.  (part of Bug 735406)

Is this :TODO: an action-item, or something that will need to be fixed when bug 735406 lands? If the latter, please clarify and remove the :TODO:.
Attachment #620937 - Flags: review?(dvander) → review+

Comment 4

5 years ago (OS/X compilation fix)
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.