1169731 - [[Call]] on a class constructor should throw

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

ES6 5/3/2015 9.2.1 step 2 reads

If F’s [[FunctionKind]] internal slot is "classConstructor", throw a TypeError exception.

Comment 1

[Eric Faust [:efaust]]

Attachment: Patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8615034 [details] [diff] [review]
Patch

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

::: js/src/jit/BaselineIC.cpp
@@ +10125,5 @@
>              return true;
>  
> +        // Likewise, if the callee is a class constructor, we have to throw.
> +        if (!constructing && fun->isClassConstructor())
> +            return true;

Baseline has a Call_AnyScripted stub that's used when we attach too many call stubs. When callee_ == nullptr in ICCallScriptedCompiler::generateStubCode, we have to do the check in JIT code. Can you add a test for this?

::: js/src/jit/CodeGenerator.cpp
@@ +3041,2 @@
>          masm.branchIfFunctionHasNoScript(calleereg, &invoke);
> +        masm.branchFunctionKind(Assembler::Equal, JSFunction::ClassConstructor, calleereg, objreg, &invoke);

Hm the overhead is probably (hopefully) negligible but if it's not, we should try to come up with a better way to test this, like have a single bit that's set when we have a script and are callable (and same for the construct case, too bad we're running out of JSFunction bits..)

::: js/src/jit/Lowering.cpp
@@ +471,5 @@
>  
>              lir = new(alloc()) LCallNative(tempFixed(cxReg), tempFixed(numReg),
>                                             tempFixed(vpReg), tempFixed(tmpReg));
> +        } else if (target->isClassConstructor() && !call->isConstructing()){
> +            lir = new(alloc()) LThrowClassConstructor();

I think it'd be simpler to just ignore the |target| in this case and fall through to the CallGeneric code, which has to deal with this case anyway.

Then you can also move the ThrowClassConstructor code in Interpreter.cpp into its single caller.

::: js/src/jit/MacroAssembler.h
@@ +946,5 @@
> +        Address flags(fun, JSFunction::offsetOfFlags());
> +        load32(flags, scratch);
> +        and32(Imm32(JSFunction::FUNCTION_KIND_MASK), scratch);
> +        rshiftPtr(Imm32(JSFunction::FUNCTION_KIND_SHIFT), scratch);
> +        branch32(cond, scratch, Imm32(kind), label);

Can we eliminate the rshiftPtr by doing something like this:

branch32(cond, scratch, Imm32(kind << JSFunction::FUNCTION_KIND_SHIFT), label);

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> like have a single bit
> that's set when we have a script and are callable (and same for the
> construct case, too bad we're running out of JSFunction bits..)

Wow, evilpie did this already for the constructor case in bug 1059908.

Anyway, your code is fine, hopefully we don't need a separate CALLABLE bit right now.

Comment 4

[Eric Faust [:efaust]]

Attachment: Patch v2

With issues addressed.

Comment 5

[Eric Faust [:efaust]]

Attachment: Patch v2

Wrong patch, sorry.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8615760 [details] [diff] [review]
Patch v2

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

::: js/src/jit/CodeGenerator.cpp
@@ +3133,5 @@
>      Label end, uncompiled;
>  
>      // Native single targets are handled by LCallNative.
>      MOZ_ASSERT(!target->isNative());
> +    MOZ_ASSERT(!target->isClassConstructor() || call->isConstructing());

Nit: personally I think this is a little clearer:

MOZ_ASSERT_IF(target->isClassConstructor(), call->isConstructing());

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f11b7896950f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f11b7896950f