The default bug view has changed. See this FAQ.

IonMonkey: Optimistic argc passed to uncompiled functions.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 619205 [details] [diff] [review]
Pass the bytecode argc value to Invoke().

Fixes all arguments-related bugs with --ion-eager. Checkem.
Attachment #619205 - Flags: review?(nicolas.b.pierron)
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".
Attachment #619205 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Comment 3

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/fb7572ed4bc6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Duplicate of this bug: 727741
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())
(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.
You need to log in before you can comment on or make changes to this bug.