Closed Bug 751635 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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)
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 on attachment 620937 [details] [diff] [review]
Create unexpected argument object from StackIter.

lgtm
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+
https://hg.mozilla.org/projects/ionmonkey/rev/7993365e3e2b
https://hg.mozilla.org/projects/ionmonkey/rev/8d5c24720c28 (OS/X compilation fix)
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.