As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 749822 - IonMonkey: Optimistic argc passed to uncompiled functions.
: IonMonkey: Optimistic argc passed to uncompiled functions.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
: 727741 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-27 15:54 PDT by Sean Stangl [:sstangl]
Modified: 2012-04-28 15:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Pass the bytecode argc value to Invoke(). (3.98 KB, patch)
2012-04-27 16:10 PDT, Sean Stangl [:sstangl]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2012-04-27 15:54:40 PDT
If Ion knows the target of an MCall, but the callsite does not pass sufficient arguments, Ion explicitly passes |Undefined| to skip the ArgumentsRectifier.

In the case where the target function has not been compiled, visitCallGeneric() will call the VM Invoke() function with an argc that includes the missing arguments. If the target function then uses |arguments|, it will create an ArgumentsObject with an incorrect length -- the |Undefined| values will be explicit instead of implicit.

We need to remember the original argc from the bytecode and pass that instead. Fixes a bunch of --ion-eager failures.
Comment 1 User image Sean Stangl [:sstangl] 2012-04-27 16:10:10 PDT
Created attachment 619205 [details] [diff] [review]
Pass the bytecode argc value to Invoke().

Fixes all arguments-related bugs with --ion-eager. Checkem.
Comment 2 User image Nicolas B. Pierron [:nbp] 2012-04-27 16:26:53 PDT
Comment on attachment 619205 [details] [diff] [review]
Pass the bytecode argc value to Invoke().

Review of attachment 619205 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I forgot to mention that vm/Stack.h use a different name for it.  The names used in vm/Stack.h are "actual" for bytecode and "formal" for the expected number.
I would prefer if you can rename "bytecodeArgc" to "actualArgc".
Comment 3 User image Sean Stangl [:sstangl] 2012-04-27 16:56:07 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/fb7572ed4bc6
Comment 4 User image Sean Stangl [:sstangl] 2012-04-27 16:57:09 PDT
*** Bug 727741 has been marked as a duplicate of this bug. ***
Comment 5 User image David Anderson [:dvander] 2012-04-27 21:26:50 PDT
Comment on attachment 619205 [details] [diff] [review]
Pass the bytecode argc value to Invoke().

Review of attachment 619205 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/MIR.h
@@ +1130,3 @@
>        : construct_(construct),
> +        target_(NULL),
> +        bytecodeArgc_(bytecodeArgc)

Drive-by comment: No need for a new field here: you can GET_UINT8(resumePoint()->pc())
Comment 6 User image Nicolas B. Pierron [:nbp] 2012-04-28 15:01:59 PDT
(In reply to David Anderson [:dvander] from comment #5)
> Comment on attachment 619205 [details] [diff] [review]
> Pass the bytecode argc value to Invoke().
> 
> Review of attachment 619205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/MIR.h
> @@ +1130,3 @@
> >        : construct_(construct),
> > +        target_(NULL),
> > +        bytecodeArgc_(bytecodeArgc)
> 
> Drive-by comment: No need for a new field here: you can
> GET_UINT8(resumePoint()->pc())

Thanks for this trick, I will apply this change as part of Bug 735406.

Note You need to log in before you can comment on or make changes to this bug.