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
:
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 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 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 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 Sean Stangl [:sstangl] 2012-04-27 16:56:07 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/fb7572ed4bc6
Comment 4 Sean Stangl [:sstangl] 2012-04-27 16:57:09 PDT
*** Bug 727741 has been marked as a duplicate of this bug. ***
Comment 5 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 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.