1270331 - Pass JS_IS_CONSTRUCTING as |this| to constructing functions from bytecode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Attachment: Patch

Currently, we push undefined and then overwrite it with something else. This is kinda lame, since it differs from the convenient C++ entry points so carefully curated by Waldo. What's more, the JITs just overwrite it with JS_IS_CONSTRUCTING already!

Let's just push it in bytecode.

Comment 1

[Jeff Walden [:Waldo]]

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

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7588,3 @@
>  
>      bool isNewOp = pn->getOp() == JSOP_NEW || pn->getOp() == JSOP_SPREADNEW ||
>                     pn->getOp() == JSOP_SUPERCALL || pn->getOp() == JSOP_SPREADSUPERCALL;;

Stray ; while you're here.

I find myself wondering if we should just do the work to push JS_UNINITIALIZED_LEXICAL for the super-cases where the caller *definitely* knows that's going to happen under the hood.  We have to overwrite it later with that, just the same way we overwrite UndefinedValue() right now.  Seems like it'd make sense to do, right?  How about a followup patch to do that as well?

::: js/src/jit/BaselineIC.cpp
@@ -6972,5 @@
>          pushSpreadCallArguments(masm, regs, argcReg, /* isJitCall = */ false, isConstructing_);
>      else
>          pushCallArguments(masm, regs, argcReg, /* isJitCall = */ false, isConstructing_);
>  
> -    if (isConstructing_) {

Can this be replaced with debug-only code asserting is-constructingness without perturbing register allocation?

@@ -7070,5 @@
>      regs.add(scratch);
>      pushCallArguments(masm, regs, argcReg, /* isJitCall = */ false, isConstructing_);
>      regs.take(scratch);
>  
> -    if (isConstructing_) {

This too.

::: js/src/jit/IonAnalysis.cpp
@@ +2527,5 @@
>        case MIRType::Value:
>        case MIRType::Float32x4:
>        case MIRType::Int32x4:
>        case MIRType::Bool32x4:
> +      case MIRType::MagicIsConstructing:

Don't wholly understand this, or why this would be treated differently from hole, but okay.

::: js/src/vm/Interpreter.cpp
@@ +344,1 @@
>          if (invoke.constructing() && invoke.args().thisv().isPrimitive()) {

Could you refactor this method a little to eliminate the massive over-indentation within it?  Also, it appears to me that the assumption is that on entry to this function, for a constructing frame, we either have an object, *or* we have JS_IS_CONSTRUCTING.  So just checking for isPrimitive() is too broad, and we should require exactly that one MagicValue in that case:

  if (!isInvoke())
    return true;

  InvokeState& invoke = *asInvoke();
  if (!invoke.isConstructing())
    return true;

  MutableHandleValue thisv = invoke.args().mutableThis();
  if (thisv.isObject())
    return true;

  MOZ_ASSERT(thisv.isMagic(JS_IS_CONSTRUCTING));
  ...rest of the body, with thisv mutated as necessary...

@@ +562,5 @@
>      MOZ_ASSERT(IsConstructor(args.CallArgs::newTarget()),
>                 "provided new.target value must be a constructor");
>  
> +    MOZ_ASSERT(args.thisv().isMagic(JS_IS_CONSTRUCTING) || args.thisv().isObject());
> +

Minor preference for putting this between the callee and newTarget checks above, so we're asserting things in stack order.

@@ +1263,5 @@
>  #define PUSH_SYMBOL(s)           REGS.sp++->setSymbol(s)
>  #define PUSH_STRING(s)           do { REGS.sp++->setString(s); assertSameCompartmentDebugOnly(cx, REGS.sp[-1]); } while (0)
>  #define PUSH_OBJECT(obj)         do { REGS.sp++->setObject(obj); assertSameCompartmentDebugOnly(cx, REGS.sp[-1]); } while (0)
>  #define PUSH_OBJECT_OR_NULL(obj) do { REGS.sp++->setObjectOrNull(obj); assertSameCompartmentDebugOnly(cx, REGS.sp[-1]); } while (0)
> +#define PUSH_MAGIC(magic)        REGS.sp++->setMagic(magic)

Purty.

::: js/src/vm/Stack.cpp
@@ +223,5 @@
>              thisArgument() = MagicValue(JS_UNINITIALIZED_LEXICAL);
>          } else if (script->isDerivedClassConstructor()) {
>              MOZ_ASSERT(callee().isClassConstructor());
>              thisArgument() = MagicValue(JS_UNINITIALIZED_LEXICAL);
> +        } else if (thisArgument().isMagic(JS_IS_CONSTRUCTING)) {

} else if (thisArgument().isObject()) {
    // Nothing to do, correctly set.
} else {
    MOZ_ASSERT(thisArgument().isMagic(JS_IS_CONSTRUCTING));
    ...

::: js/src/vm/Stack.h
@@ +1020,5 @@
>      using Base = detail::FixedArgsBase<CONSTRUCT, N>;
>  
>    public:
> +    explicit FixedConstructArgs(JSContext* cx) : Base(cx) {
> +        this->CallArgs::setThis(MagicValue(JS_IS_CONSTRUCTING));

This makes more sense in FixedArgsBase, just after that method's |this->constructing_ = Constructing;|, for consistency with the other modification being to GenericArgsBase rather than to ConstructArgs.  Right?

Comment 2

[Jan de Mooij [:jandem]]

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

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

Nice idea.

In IonBuilder::createThis, when we have a native JSFunction, we do:

  MConstant* magic = MConstant::New(alloc(), MagicValue(JS_IS_CONSTRUCTING));

I think we can now pass callInfo.thisArg() to createThis(), and return that (and assert it has the is-constructing MIRType if possible). That way we don't create an extra MConstant when we have a native call.

::: js/src/vm/Opcodes.h
@@ +612,5 @@
> +     *   Type: Constants
> +     *   Operands:
> +     *   Stack: => JS_IS_CONSTRUCTING
> +     */ \
> +    macro(JSOP_IS_CONSTRUCTING,  65, "is-constructing",   NULL,         1,  0,  1, JOF_BYTE) \

Don't forget to bump the XDR constant. Oh wait.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38cb4f9e6f31
Pass JS_IS_CONSTRUCTING as |this| to constructing functions from bytecode. (r=Waldo, r=jandem)

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38cb4f9e6f31