Pass JS_IS_CONSTRUCTING as |this| to constructing functions from bytecode

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8748971 [details] [diff] [review]
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.
Attachment #8748971 - Flags: review?(jwalden+bmo)
Attachment #8748971 - Flags: review?(jdemooij)
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?
Attachment #8748971 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8748971 - Flags: review?(jdemooij) → review+

Comment 3

2 years ago
Pushed by efaustbmo@gmail.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38cb4f9e6f31
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.