IonMonkey: Handle function calls with insufficient arguments.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Created attachment 558752 [details]
Test case for insufficient arguments.

The idea is to have compartment-wide code, in generated asm, that creates a new stack frame below the existing stack frame with insufficient arguments; this thunk is then called instead of the actual function.

By handling this logic in the caller, there is no penalty for the case in which the number of arguments for the callee is known at compile-time.
(Reporter)

Comment 1

7 years ago
Created attachment 560012 [details] [diff] [review]
Call functions with insufficient arguments.
Attachment #560012 - Flags: review?(dvander)
Comment on attachment 560012 [details] [diff] [review]
Call functions with insufficient arguments.

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

Patch looks good but we can't tell frame types apart yet and we should figure that out first.

::: js/src/ion/Ion.cpp
@@ +139,5 @@
>    : execAlloc_(NULL),
>      enterJIT_(NULL),
>      bailoutHandler_(NULL),
> +    returnError_(NULL),
> +    argumentsRectifier_(NULL)

In IonCompartment::sweep() make sure to check whether argumentsRectifier_ is dead and should be NULL'd.

::: js/src/ion/IonFrames.h
@@ +63,4 @@
>  
>  // Layout of the frame prefix. This assumes the stack architecture grows down.
>  // If this is ever not the case, we'll have to refactor.
> +struct IonFrameData

We need a way to differentiate rectifier frames from normal frames. The size descriptor bit alone isn't enough - should we steal another bit?

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +743,5 @@
> +    {
> +        Label thunk, rejoin;
> +
> +        // Get the address of the argumentsRectifier code.
> +        IonCompartment *ion = gen->cx->compartment->ionCompartment();

Can we bite the bullet and make an easier way to access this? like GetIonCompartment or just gen->compartment ?

@@ +749,5 @@
> +        if (!argumentsRectifier)
> +            return false;
> +
> +        // Check whether the provided arguments satisfy target argc.
> +        masm.load16(Operand(tokreg, offsetof(JSFunction, nargs)), nargsreg);

Should load16 be the actual mnemonic, or is load16 in MacroAssembler?

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +196,5 @@
>      return linker.newCode(cx);
>  }
>  
> +IonCode *
> +IonCompartment::generateArgumentsRectifier(JSContext *cx)

Comments here apply to x64 too:

@@ +202,5 @@
> +    MacroAssembler masm(cx);
> +
> +    // ArgumentsRectifierReg contains the |nargs| pushed onto the current frame.
> +    // Including |this|, there are (|nargs| + 1) arguments to copy.
> +    JS_ASSERT(ArgumentsRectifierReg == esi);

const Register nargs = ArgumentsRectifierReg? then no need to hardcode esi

@@ +210,5 @@
> +    masm.load16(Operand(eax, offsetof(JSFunction, nargs)), ecx);
> +    masm.subl(esi, ecx);
> +
> +    masm.movl(Imm32(JSVAL_TAG_UNDEFINED), ebx); // ebx <- type(undefined)
> +    masm.xorl(edi, edi);                        // edi <- payload(undefined)

Any way to common this out with the code in CodeGeneratorX86::visitValue(LValue *value)?

Something like MacroAssembler::moveValue? (in the "internal" section)
Attachment #560012 - Flags: review?(dvander)
(Reporter)

Updated

7 years ago
Blocks: 686607
(Reporter)

Comment 3

7 years ago
(In reply to David Anderson [:dvander] from comment #2)
> We need a way to differentiate rectifier frames from normal frames. The size
> descriptor bit alone isn't enough - should we steal another bit?

Taking another bit seems to be the most reasonable choice. The overhead is pretty low, and it's better than taking up extra memory on x64, were we do use some padding in the IonFrameData.

> Can we bite the bullet and make an easier way to access this? like
> GetIonCompartment or just gen->compartment ?

Something like gen->ionCompartment() sounds OK, but the organization of what class feeds into what else is nonsensical. Chris expressed some desire to clean this up later.

> Should load16 be the actual mnemonic, or is load16 in MacroAssembler?

It's in JSC's MacroAssembler.

> const Register nargs = ArgumentsRectifierReg? then no need to hardcode esi

I went with an assert over the above because even the knowledge that ArgumentsRectifierReg == esi is hardcoded in either case, and I would rather be explicit -- even if it's called ArgumentsRectifierReg, the rest of the code has to be very careful to not accidentally use esi. Such an error is less likely to occur if registers are explicit.
(Reporter)

Comment 4

7 years ago
Created attachment 560244 [details] [diff] [review]
Mark frame types correctly.

This patch correctly marks the FrameType in the IonFramePrefix by stealing yet another bit from the IonFrameData.sizeDescriptor. The flavors of FrameTypes are expressed by an enum in IonFramePrefix.

There is one oddity in the patch: since the EntryFrame never has its sizeDescriptor read, and since the sizeDescriptor is always at least 4-byte aligned, we don't need to bother with shifting in generateEnterJIT(). The patch just uses orl() to set the EnterFrameType flag, then xorl() to unset later, saving us two ops.
Attachment #560244 - Flags: review?(dvander)
Comment on attachment 560244 [details] [diff] [review]
Mark frame types correctly.

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

::: js/src/ion/IonFrames.h
@@ +75,5 @@
>    public:
> +    enum FrameType {
> +        JSFrameType,
> +        EntryFrameType,
> +        RectifierFrameType

I think you can lose the "Type" suffix
Attachment #560244 - Flags: review?(dvander) → review+
(Reporter)

Comment 6

7 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/31641ffb0e9d
http://hg.mozilla.org/projects/ionmonkey/rev/23edb5d4dea5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.