1141865 - (new.target) Implement ES6 NewTarget

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Constructors now get the constructor initially invoked with |new| made available to them via new syntax: new.target.

This is useful for implementing super(), which forwards this information to the super-constructor.

Comment 1

[Tom S [:evilpie]]

This is not necessarily exclusive to classes, right?

Comment 2

[Tom S [:evilpie]]

i.e. See https://bugzilla.mozilla.org/show_bug.cgi?id=1138157#c3

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 1: Make two ICCall_Fallback stubs, one for NEW and one not.

This absolutely should not land without the rest of it, as it makes no sense to do presently. It will make the baseline bits of newtarget much more palatable, though, and is in a reviewable state.

Comment 4

[Eric Faust [:efaust]]

Attachment: WIP: Snake newTarget through the system. Spread ops NYI.

No spread support yet, but there was a lot to dig into here, and passing jit-tests is a good time to have someone take a look at big stuff like this. I have more split up versions of this patch, if people want to see it in smaller pieces.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8592005 [details] [diff] [review]
Part 1: Make two ICCall_Fallback stubs, one for NEW and one not.

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

Looks good, this definitely seems simpler.

::: js/src/jit/BaselineBailouts.cpp
@@ +453,5 @@
>      if (IsSetPropPC(pc))
>          return cx->compartment()->jitCompartment()->baselineSetPropReturnAddr();
>      // This should be a call op of some kind, now.
>      MOZ_ASSERT(IsCallPC(pc));
> +    return cx->compartment()->jitCompartment()->baselineCallReturnAddr(JSOp(*pc) == JSOP_NEW);

Ion can't compile the SPREAD* ops yet so SPREADNEW can't show up here, but let's assert that:

MOZ_ASSERT(*pc != JSOP_SPREADNEW);

::: js/src/jit/BaselineIC.cpp
@@ +10606,5 @@
>  
>      // Load passed-in ThisV into R1 just in case it's needed.  Need to do this before
>      // we leave the stub frame since that info will be lost.
>      // Current stack:  [...., ThisV, ActualArgc, CalleeToken, Descriptor ]
>      masm.loadValue(Address(BaselineStackReg, 3 * sizeof(size_t)), R1);

Nit: we should only do this |if (isConstructing_)|.

::: js/src/jit/BaselineIC.h
@@ +5490,5 @@
>      static const uint32_t MAX_SCRIPTED_STUBS = 7;
>      static const uint32_t MAX_NATIVE_STUBS = 7;
>    private:
>  
> +    ICCall_Fallback(JitCode* stubCode)

Nit: this constructor should be |explicit| now.

@@ +5530,5 @@
>          bool generateStubCode(MacroAssembler& masm);
>          bool postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code);
>  
>          virtual int32_t getKey() const {
> +            return static_cast<int32_t>(kind) | (static_cast<int32_t>(isSpread_) << 16) |

Maybe add a brief comment here to remind the reader to update CALL_KEY/CONSTRUCT_KEY if necessary.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8592006 [details] [diff] [review]
WIP: Snake newTarget through the system. Spread ops NYI.

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

Nice work, just skimmed the patch but looks good.

::: js/src/jit/BaselineCompiler.cpp
@@ +2738,5 @@
>  BaselineCompiler::emitCall()
>  {
>      MOZ_ASSERT(IsCallPC(pc));
>  
> +    bool construct = JSOp(*pc) == JSOP_NEW;

Nit: I think most places call this "constructing", would be nice to be consistent.

::: js/src/jit/BaselineIC.cpp
@@ +10285,5 @@
> +    // If we are setting up for a jitcall, we have to align the stack taking
> +    // into account the args and newTarget. We could also count callee and |this|,
> +    // but it's a waste of stack space. Because we want to keep argcReg unchanged,
> +    // just account for newTarget initially, and add the other 2 after assuring
> +    // allignment.

Nit: alignment, typo

@@ +10758,3 @@
>  
>          // Stack now looks like:
>          //      [..., Callee, ThisV, Arg0V, ..., ArgNV, StubFrameHeader, ArgC ]

Nit: update the comment(s)

@@ +11099,1 @@
>          masm.loadValue(calleeSlot, R1);

To test the CallNative case, maybe add an assert to js::ArrayConstructor and see if it passes jit-tests:

MOZ_ASSERT(newTarget->as<JSFunction>().native() == ArrayConstructor);

::: js/src/jit/CodeGenerator.cpp
@@ +3114,5 @@
>  
>      // Handle uncompiled functions.
>      masm.bind(&uncompiled);
> +    if (call->isConstructing() && target->nargs() > call->numActualArgs())
> +        emitCallInvokeFunctionShuffleNewTarget(call, calleereg, target->nargs(), unusedStack);

Hm shouldn't we have padded any missing arguments with |undefined| in the CallKnown case?

::: js/src/jit/IonBuilder.cpp
@@ +6277,5 @@
>              isDOMCall = true;
>          }
>      }
>  
> +    MCall* call = MCall::New(alloc(), target, targetArgs + 1 + callInfo.constructing(),

I think we should make sure Ion can inline functions that use new.target, to make sure inlining passes it through correctly.

::: js/src/jit/IonBuilder.h
@@ +1346,5 @@
> +    void setNewTarget(MDefinition* newTarget) {
> +        MOZ_ASSERT(constructing());
> +        newTargetArg_ = newTarget;
> +    }
> +    MDefinition *getNewTarget() const {

Nit: MDefinition* get..

::: js/src/vm/Interpreter.cpp
@@ +795,5 @@
>  
>      args.setThis(MagicValue(JS_IS_CONSTRUCTING));
>  
>      if (!args.calleev().isObject())
> +        return ReportIsNotFunction(cx, args.calleev(), args.length() + 2, CONSTRUCT);

The +2's in this function could use a comment, as it usually means "callee + this" but here it's "this + newTarget".

Comment 7

[Eric Faust [:efaust]]

WIP with spread on try at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a59baa3719af

I suspect we will have InvokeConstructor problems eventually, with forwarding newTarget instead of instantiating it properly, but I want to get a baseline for mochitests.

--tbpl jit tests and jstests both pass locally.

Comment 8

[Eric Faust [:efaust]]

Attachment: WIP: base (part 1 of brokenness)

Uploading to get some debugging help

Comment 9

[Eric Faust [:efaust]]

Attachment: WIP: breaking (brokenness pt 2)

Comment 10

[Eric Faust [:efaust]]

Attachment: Part 2: Plumbing for new.target

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 3: Implement proper new.target forwarding semantics for inlined bound functions in ion.

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 4: implement scripted proxy new.target trap argument

Comment 13

[Eric Faust [:efaust]]

Attachment: Part 5: new.target parser support, reflect support, and tests

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8604279 [details] [diff] [review]
Part 2: Plumbing for new.target

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

::: js/src/jit/Ion.cpp
@@ +2424,5 @@
>              data.maxArgv = args.base() + 1;
>          } else {
>              MOZ_ASSERT(vals.empty());
> +            unsigned numPushedArgs = Max(args.length(), numFormals);
> +            if (!vals.reserve(numPushedArgs + 1 + data.constructing))

Self-nit: We also need a modification like this in EnterBaselineAtBranch()

I think 
diff --git a/js/src/jit/BaselineJIT.cpp b/js/src/jit/BaselineJIT.cpp
--- a/js/src/jit/BaselineJIT.cpp
+++ b/js/src/jit/BaselineJIT.cpp
@@ -181,17 +181,19 @@ jit::EnterBaselineAtBranch(JSContext* cx
     data.osrFrame = fp;
     data.osrNumStackValues = fp->script()->nfixed() + cx->interpreterRegs().stackDepth();
 
     RootedValue thisv(cx);
 
     if (fp->isNonEvalFunctionFrame()) {
         data.constructing = fp->isConstructing();
         data.numActualArgs = fp->numActualArgs();
-        data.maxArgc = Max(fp->numActualArgs(), fp->numFormalArgs()) + 1; // +1 = include |this|
+
+        // Include |this| and possible |newTarget|
+        data.maxArgc = Max(fp->numActualArgs(), fp->numFormalArgs()) + 1 + data.constructing;
         data.maxArgv = fp->argv() - 1; // -1 = include |this|
         data.scopeChain = nullptr;
         data.calleeToken = CalleeToToken(&fp->callee(), data.constructing);
     } else {
         thisv = fp->thisValue();
         data.constructing = false;
         data.numActualArgs = 0;
         data.maxArgc = 1;

should be sufficient.

Comment 15

[Eric Faust [:efaust]]

(In reply to Eric Faust [:efaust] from comment #14)
> Comment on attachment 8604279 [details] [diff] [review]
> Part 2: Plumbing for new.target
> 
> Review of attachment 8604279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Ion.cpp
> @@ +2424,5 @@
> >              data.maxArgv = args.base() + 1;
> >          } else {
> >              MOZ_ASSERT(vals.empty());
> > +            unsigned numPushedArgs = Max(args.length(), numFormals);
> > +            if (!vals.reserve(numPushedArgs + 1 + data.constructing))
> 
> Self-nit: We also need a modification like this in EnterBaselineAtBranch()
> 
> I think 
> diff --git a/js/src/jit/BaselineJIT.cpp b/js/src/jit/BaselineJIT.cpp
> --- a/js/src/jit/BaselineJIT.cpp
> +++ b/js/src/jit/BaselineJIT.cpp
> @@ -181,17 +181,19 @@ jit::EnterBaselineAtBranch(JSContext* cx
>      data.osrFrame = fp;
>      data.osrNumStackValues = fp->script()->nfixed() +
> cx->interpreterRegs().stackDepth();
>  
>      RootedValue thisv(cx);
>  
>      if (fp->isNonEvalFunctionFrame()) {
>          data.constructing = fp->isConstructing();
>          data.numActualArgs = fp->numActualArgs();
> -        data.maxArgc = Max(fp->numActualArgs(), fp->numFormalArgs()) + 1;
> // +1 = include |this|
> +
> +        // Include |this| and possible |newTarget|
> +        data.maxArgc = Max(fp->numActualArgs(), fp->numFormalArgs()) + 1 +
> data.constructing;
>          data.maxArgv = fp->argv() - 1; // -1 = include |this|
>          data.scopeChain = nullptr;
>          data.calleeToken = CalleeToToken(&fp->callee(), data.constructing);
>      } else {
>          thisv = fp->thisValue();
>          data.constructing = false;
>          data.numActualArgs = 0;
>          data.maxArgc = 1;
> 
> should be sufficient.

This is obviously nonsense. Please ignore.

What I /did/ find, however, was a bug in bailouts with rectifier frames. Patch with test in next upload.

Comment 16

[Eric Faust [:efaust]]

Attachment: Part 2.1: Copy new.target value into bailout-created rectifier frame

Comment 17

[Eric Faust [:efaust]]

Attachment: Part 2.2 Ensure we get correct overflow arguments in RematerializedFrames.

This is probably the right person to flag for review for this patch, though it's hard to put it into the overall context. Feel free to forward.

Comment 18

[Eric Faust [:efaust]]

Attachment: Part 2.2 Ensure we get correct overflow arguments in RematerializedFrames.

Clean up the test a little ;)

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8604943 [details] [diff] [review]
Part 2.2 Ensure we get correct overflow arguments in RematerializedFrames.

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

Thanks for the fix!

::: js/src/jit/JitFrameIterator.h
@@ +748,5 @@
>                      SnapshotIterator parent_s(it.snapshotIterator());
>  
>                      // Skip over all slots until we get to the last slots
>                      // (= arguments slots of callee) the +3 is for [this], [returnvalue],
>                      // [scopechain], and maybe +1 for [argsObj]

Please update the comment as well.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8604279 [details] [diff] [review]
Part 2: Plumbing for new.target

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

This looks OK but I posted some nits/comments in comment 6; can you please address these first?

::: js/public/CallArgs.h
@@ +325,5 @@
>      // here than we'd like (because they're hackish and drop assertions).  Try
>      // to avoid using these if you can.
>  
>      Value* array() const { return this->argv_; }
> +    Value* end() const { return this->argv_ + argc_ + constructing_; }

Please check the build log of a Windows Try build with and without this patch to make sure it doesn't add a ton of warnings about the many "pointer + bool" or "unsigned + bool" expressions this patch adds.

::: js/src/jit/BaselineBailouts.cpp
@@ +976,5 @@
>              BailoutKindString(bailoutKind));
>  #endif
>  
> +    bool pushedNewTarget = op == JSOP_NEW;
> +    

Nit: trailing whitespace.

::: js/src/jit/BaselineIC.cpp
@@ +10517,5 @@
> +    // If we are setting up for a jitcall, we have to align the stack taking
> +    // into account the args and newTarget. We could also count callee and |this|,
> +    // but it's a waste of stack space. Because we want to keep argcReg unchanged,
> +    // just account for newTarget initially, and add the other 2 after assuring
> +    // allignment.

Nit: alignment, typo

@@ +10583,4 @@
>      Register startReg = regs.takeAny();
> +    masm.unboxObject(Address(BaselineStackReg, (isConstructing * sizeof(Value)) + STUB_FRAME_SIZE), startReg);
> +    masm.loadPtr(Address(startReg, NativeObject::offsetOfElements()), startReg);
> +    

Trailing whitespace.

@@ +10830,5 @@
>      if (MOZ_UNLIKELY(isSpread_)) {
>          // Use BaselineFrameReg instead of BaselineStackReg, because
>          // BaselineFrameReg and BaselineStackReg hold the same value just after
>          // calling enterStubFrame.
> +        

Trailing whiteespace here and 3 times below.

::: js/src/jit/CodeGenerator.cpp
@@ +3104,5 @@
>      masm.jump(&end);
>  
>      // Handle uncompiled functions.
>      masm.bind(&uncompiled);
> +    if (call->isConstructing() && target->nargs() > call->numActualArgs())

I also commented this on the WIP patch (please make sure these comments are all addressed), but shouldn't we have padded any missing arguments with |undefined| in the CallKnown case?

::: js/src/jit/CodeGenerator.h
@@ +134,2 @@
>      void visitCallGeneric(LCallGeneric* call);
> +    void emitCallInvokeFunctionShuffleNewTarget(LCallKnown *call,

Nit: LCallKnown* call

Comment 21

[Eric Faust [:efaust]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> ::: js/src/jit/CodeGenerator.cpp
> @@ +3104,5 @@
> >      masm.jump(&end);
> >  
> >      // Handle uncompiled functions.
> >      masm.bind(&uncompiled);
> > +    if (call->isConstructing() && target->nargs() > call->numActualArgs())
> 
> I also commented this on the WIP patch (please make sure these comments are
> all addressed), but shouldn't we have padded any missing arguments with
> |undefined| in the CallKnown case?

Yeah, but that's exactly the point. What's going on here is kind of subtle, and definitely silly.

InvokeConstructor() expects the newTarget value to be tight at the end of the argument vector.

LCallKnown::numActualArgs() calls MCall::numActualArgs(), which is filled in from CallInfo::argc() in makeCallHelper (https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#6279). This check looks explicitly for this case, and calls a different VMFunction which wiggles the newTarget back to being dense against the arg vector to call back into the VM.

Is there something else in this concern that I'm missing?

I will certainly go back and update all the stack diagrams, and make sure that the rest of those concerns are addressed.

Comment 22

[Eric Faust [:efaust]]

See comment 21

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #21)
> Yeah, but that's exactly the point. What's going on here is kind of subtle,
> and definitely silly.
> 
> InvokeConstructor() expects the newTarget value to be tight at the end of
> the argument vector.

Ah, makes sense. I thought this case was unreachable, but it isn't because padding the missing arguments does not bump argc.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8604279 [details] [diff] [review]
Part 2: Plumbing for new.target

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

Looks great! Comments are mostly nits but I'd like to see the final patch with these addressed; can review it quickly.

::: js/src/jit/IonBuilder.h
@@ +1363,5 @@
> +    void setNewTarget(MDefinition* newTarget) {
> +        MOZ_ASSERT(constructing());
> +        newTargetArg_ = newTarget;
> +    }
> +    MDefinition *getNewTarget() const {

MDefinition* getNewTarget

::: js/src/jit/LIR-Common.h
@@ +1532,5 @@
>      // Does not include |this|.
>      uint32_t numActualArgs() const {
>          return mir()->numActualArgs();
>      }
> +    

Trailing whitespace.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +152,5 @@
>  
> +    {
> +        Label noNewTarget;
> +        masm.test32(r9, Imm32(CalleeToken_FunctionConstructing));
> +        masm.j(Assembler::Zero, &noNewTarget);

Nit: slight preference for the higher-level variant, so the code looks similar for each platform.

masm.branchTest32(Assembler::Zero, r9, Imm32(CalleeToken_FunctionConstructing), &noNewTarget);

Same for the other archs.

@@ +154,5 @@
> +        Label noNewTarget;
> +        masm.test32(r9, Imm32(CalleeToken_FunctionConstructing));
> +        masm.j(Assembler::Zero, &noNewTarget);
> +
> +        masm.ma_add(r1, Imm32(1), r1);

masm.add32(Imm32(1), r1);

@@ +480,5 @@
> +    {
> +        Label notConstructing;
> +
> +        masm.test32(r1, Imm32(CalleeToken_FunctionConstructing));
> +        masm.j(Assembler::Zero, &notConstructing);

And here.

@@ +488,5 @@
> +        masm.ma_dataTransferN(IsStore, 64, true, sp, Imm32(-8), r4, PreIndex);
> +
> +        // Include the newly pushed newTarget value in the frame size
> +        // calculated below.
> +        masm.ma_add(r6, Imm32(1), r6);

And here.

@@ +492,5 @@
> +        masm.ma_add(r6, Imm32(1), r6);
> +
> +        masm.bind(&notConstructing);
> +    }
> +    

Nit: trailing whitespace

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +69,2 @@
>      masm.loadPtr(Address(ebp, ARG_ARGC), eax);
> +    

Trailing whitespace here and below.

@@ +410,5 @@
>        "Ensure that we can pad the stack by pushing extra UndefinedValue");
>  
>      MOZ_ASSERT(IsPowerOfTwo(JitStackValueAlignment));
>      masm.addl(Imm32(JitStackValueAlignment - 1 /* for padding */), ecx);
> +    

And here.

@@ +414,5 @@
> +    
> +    // Account for newTarget, if necessary.
> +    masm.mov(eax, edx);
> +    masm.andl(Imm32(CalleeToken_FunctionConstructing), edx);
> +    masm.addl(edx, ecx);

We should static_assert CalleeToken_FunctionConstructing == 1 here, same for the other architectures.

@@ +477,5 @@
> +        Label notConstructing;
> +
> +        masm.mov(eax, ebx);
> +        masm.andl(Imm32(CalleeToken_FunctionConstructing), ebx);
> +        masm.j(AssemblerX86Shared::Zero, &notConstructing);

This can be branchTest32, afaik ebx is not used afterwards.

@@ +484,5 @@
> +                           sizeof(RectifierFrameLayout) +
> +                           sizeof(Value) +
> +                           sizeof(void*));
> +
> +        masm.mov(eax, ebx);

And then we don't need this move.

::: js/src/jsarray.cpp
@@ +3151,5 @@
>  bool
>  js::ArrayConstructor(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    

Nit: trailing whitespace here and below.

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +304,5 @@
>              if (!cx->compartment()->wrap(cx, args[n]))
>                  return false;
>          }
> +        if (!cx->compartment()->wrap(cx, args.newTarget()))
> +            return false;

Please make sure we have tests for this.

::: js/src/proxy/DirectProxyHandler.cpp
@@ +81,5 @@
>  DirectProxyHandler::construct(JSContext* cx, HandleObject proxy, const CallArgs& args) const
>  {
>      assertEnteredPolicy(cx, proxy, JSID_VOID, CALL);
>      RootedValue target(cx, proxy->as<ProxyObject>().private_());
> +    return InvokeConstructor(cx, target, args.length(), args.array(), true, args.rval());

Maybe make this bool an enum or enum class, or add /* newTargetInArgv = */ comments.

::: js/src/vm/Interpreter.cpp
@@ +792,5 @@
>  js::InvokeConstructor(JSContext* cx, CallArgs args)
>  {
>      MOZ_ASSERT(!JSFunction::class_.construct);
>  
>      args.setThis(MagicValue(JS_IS_CONSTRUCTING));

Can we MOZ_ASSERT(args.newTarget().isObjectOrNull()); here?

@@ +795,5 @@
>  
>      args.setThis(MagicValue(JS_IS_CONSTRUCTING));
>  
>      if (!args.calleev().isObject())
> +        return ReportIsNotFunction(cx, args.calleev(), args.length() + 2, CONSTRUCT);

As mentioned in comment 6, it'd be good to explain the +2 is for |this| and new.target.

::: js/src/vm/Opcodes.h
@@ +389,5 @@
>       * Invokes 'callee' as a constructor with 'this' and 'args', pushes the
>       * return value onto the stack.
>       *   Category: Statements
>       *   Type: Function
>       *   Operands:

Please bump XDR_BYTECODE_VERSION_SUBTRAHEND when modifying the bytecode.

::: js/src/vm/Stack-inl.h
@@ +286,3 @@
>          *pargv = args.array();
>          uint8_t* buffer = allocateFrame(cx, sizeof(InterpreterFrame) + nvals * sizeof(Value));
>          return reinterpret_cast<InterpreterFrame*>(buffer);

Please make sure we have tests for new.target with nactuals < nformals and nactuals > nformals.

Comment 25

[Eric Faust [:efaust]]

Attachment: Part 6: Support new.target in eval frames.

Comment 26

[Eric Faust [:efaust]]

Attachment: Part 7: Support new.target inside arrow functions

Comment 27

[Eric Faust [:efaust]]

Attachment: Part 8: Support new.target in generator functions.

For the life of me I do not understand why this is specced to work, but it is, so we dutifully walk off the cliff with the other lemmings.

Comment 28

[Eric Faust [:efaust]]

Attachment: Part 2.3: Address the nits from Jan's first review.

There are more tests (including tests for both argument over and underflow) in part 5, as well as a subtrahend increment. Since it's hard to test the plumbing without the parser support, I figured I would land them together.

Comment 29

[Eric Faust [:efaust]]

Attachment: Part 7.1: remove some trailing whitespace, and add forgotten test for arrow functions.

Comment 30

[Jason Orendorff [:jorendorff]]

Comment on attachment 8604279 [details] [diff] [review]
Part 2: Plumbing for new.target

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

r=me on the non-jit parts, with comments.

::: js/public/CallArgs.h
@@ +284,5 @@
>                                      CallReceiverBase<NoUsedRval> >::Type
>  {
>    protected:
>      unsigned argc_;
> +    bool constructing_;

Would it be bad to put this field in CallReceiverBase? I ask because the isConstructing() method there is pretty sketchy compared to just testing a field.

Just a thought; feel free to disgregard.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6440,5 @@
>      } else {
>          if (!emitArray(pn2->pn_next, argc, JSOP_SPREADCALLARRAY))
>              return false;
> +
> +        if (isNewOp) {

Hmm. I'm not sure I would have added this part, since JSOP_NEW_SPREAD shouldn't have "false degrees of freedom" i.e. a parameter that's always predictable from the other parameters in practice. I think the changes to CallSpreadOperation and so on would have been smaller that way.

(JSOP_NEW is different: there the extra argument serves a purpose, in the interpreter at least.)

::: js/src/jsfun.cpp
@@ +1567,5 @@
>      if (!constructing)
>          invokeArgs.setThis(boundThis);
>  
> +    if (constructing) {
> +        if (&args.newTarget().toObject() == fun)

Comment that this is 9.4.1.2 step 5?

::: js/src/jsiter.cpp
@@ +452,2 @@
>           * We are always coming from js::ValueToIterator, and we are no longer on
>           * trace, so the object we are iterating over is on top of the stack (-1).

Please update this old comment that uses the phrase "on trace" (good grief).

@@ +456,5 @@
>          if (!AtomToPrintableString(cx, name, &bytes))
>              return false;
>          RootedValue val(cx, ObjectValue(*obj));
>          ReportValueError2(cx, JSMSG_BAD_TRAP_RETURN_VALUE,
> +                          JSDVG_IGNORE_STACK, val, js::NullPtr(), bytes.ptr());

Is this related to the rest of this patch? It looks like for once the -1 here was correct.

::: js/src/vm/Opcodes.h
@@ +390,5 @@
>       * return value onto the stack.
>       *   Category: Statements
>       *   Type: Function
>       *   Operands:
> +     *   Stack: callee, this, args, newTarget => rval

The comment for JSOP_NEW needs an update too.

::: js/src/vm/Stack-inl.h
@@ +298,5 @@
>      uint8_t* buffer = allocateFrame(cx, sizeof(InterpreterFrame) + nvals * sizeof(Value));
>      if (!buffer)
>          return nullptr;
>  
>      Value* argv = reinterpret_cast<Value*>(buffer);

I'm surprised to see this in the existing code. Doesn't argv usually mean a pointer to the first argument? We usually use vp for a pointer to the callee/eturn-value slot.

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 8604281 [details] [diff] [review]
Part 4: implement scripted proxy new.target trap argument

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

::: js/src/jit-test/tests/proxy/testDirectProxyConstruct2.js
@@ +13,5 @@
>          assertEq(target1, target);
>          assertEq(args.length, 2);
>          assertEq(args[0], 2);
>          assertEq(args[1], 3);
> +        assertEq(newTarget, p);

Maybe add:
        assertEq(new.target, undefined);
to confirm that the construct trap is [[Call]]ed, not [[Construct]]ed.

Comment 32

[Jan de Mooij [:jandem]]

Comment on attachment 8604280 [details] [diff] [review]
Part 3: Implement proper new.target forwarding semantics for inlined bound functions in ion.

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

Sorry for the delay. This patch is not unreasonable, but I think it'd be simpler and justified to just abort inlining of bound function calls if newTarget != boundFunction. WDYT?

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8608959 [details] [diff] [review]
Part 6: Support new.target in eval frames.

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

r=me with the issues below addressed.

::: js/src/builtin/Eval.cpp
@@ +462,5 @@
>              return false;
>          nthisValue = ObjectValue(*obj);
>      }
>  
> +    return ExecuteKernel(cx, esg.script(), *scopeobj, nthisValue, newTargetValue, 

Trailing whitespace.

::: js/src/frontend/SharedContext.h
@@ +232,5 @@
>  
> +    enum AllowedSyntax {
> +        AllowSyntax_NewTarget,
> +        AllowSyntax_SuperProperty
> +    };

This wants an enum class:

enum class AllowedSyntax {
    NewTarget,
    SuperProperty
};

::: js/src/jit/BaselineFrame.h
@@ +223,5 @@
> +        return (Value*)(reinterpret_cast<const uint8_t*>(this) +
> +                        BaselineFrame::Size() +
> +                        offsetOfArg(0));
> +    }
> + 

Trailing whitespace.

@@ +417,5 @@
>      }
>      static size_t offsetOfThis() {
>          return FramePointerOffset + js::jit::JitFrameLayout::offsetOfThis();
>      }
> +    static size_t offsetOfEvalNewTarget() { 

Trailing whitespace.

::: js/src/jit/BaselineJIT.cpp
@@ +208,5 @@
> +        if (fp->isEvalFrame()) {
> +            if (!vals.reserve(2))
> +                return JitExec_Aborted;
> +
> +            vals.infallibleAppend(thisv);

Hm why do we need to push |this| here now and not before? Is this value used anywhere? Do we have to trace it in BaselineFrame::trace?

@@ +209,5 @@
> +            if (!vals.reserve(2))
> +                return JitExec_Aborted;
> +
> +            vals.infallibleAppend(thisv);
> +            

Trailing whitespace.

::: js/src/jit/Ion.cpp
@@ +2460,5 @@
>          {
>              ScriptFrameIter iter(cx);
>              if (iter.isFunctionFrame())
>                  data.calleeToken = CalleeToToken(iter.callee(cx), /* constructing = */ false);
> +                

Trailing whitespace and twice below.

@@ +2467,5 @@
> +                return false;
> +            
> +            data.maxArgc = 2;
> +            data.maxArgv = vals.begin();
> +            vals.infallibleAppend(state.asExecute()->thisv());

Same question here; why do we have to push |this| now and not before?

::: js/src/jit/Lowering.cpp
@@ +627,2 @@
>          useBoxAtStart(lir, LCallDirectEvalV::ThisValue, thisValue);
> +        useBoxAtStart(lir, LCallDirectEvalV::NewTarget, newTargetValue);

If the profiler is enabled, the frame pointer register is not available. That leaves 6 registers on x86; I think this patch needs 7 in the CallDirectEvalV case:

- 2 for LCallDirectEvalV::ThisValue
- 2 for LCallDirectEvalV::NewTarget
- 2 for LCallDirectEvalV::Argument
- 1 for the scope chain

Please write a shell test that hits this (it should assert in the register allocator I think) and fix it. I don't see an obvious fix; maybe we can use stack allocation for the scope chain: s/useRegisterAtStart/useAtStart/, or get rid of LCallDirectEvalV; not sure when we use that one instead of LCallDirectEvalS.

::: js/src/tests/ecma_6/Class/newTargetEval.js
@@ +7,5 @@
> +// new.target is (for now!) invalid inside arrow functions, or eval inside arrow
> +// functions.
> +assertThrowsInstanceOf(() => eval('new.target'), SyntaxError);
> +
> +function assertNewTarget(expected) { assertEq(eval('new.target'), expected); }

Please add similar tests for *nested* eval in global/function/arrow scope:

eval("eval('new.target')");
eval("eval(\"eval('new.Target')\")")

We probably also need some tests for indirect eval.

::: js/src/vm/Stack.cpp
@@ +43,5 @@
>       */
>      flags_ = type | HAS_SCOPECHAIN;
>  
>      JSObject* callee = nullptr;
> +    RootedValue newTarget(cx, newTargetValue);

Maybe use a plain, unrooted Value, also because of the unrooted callee above. Your call.

@@ +69,5 @@
>                  flags_ |= GLOBAL;
>              }
>          }
>      }
> +    

Trailing whitespace.

Comment 34

[Jan de Mooij [:jandem]]

Comment on attachment 8608962 [details] [diff] [review]
Part 7: Support new.target inside arrow functions

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5569,5 @@
>      unsigned index = objectList.add(pn->pn_funbox);
>  
>      /* Non-hoisted functions simply emit their respective op. */
>      if (!pn->functionIsHoisted()) {
>          /* JSOP_LAMBDA_ARROW is always preceded by JSOP_THIS. */

... and JSOP_NEWTARGET.

::: js/src/jit/BaselineCompiler.cpp
@@ +2707,5 @@
> +        masm.loadValue(Address(scratch, FunctionExtended::offsetOfArrowNewTargetSlot()), R0);
> +        frame.push(R0);
> +        return true;
> +    }
> +        

Whitespace.

::: js/src/jit/IonBuilder.cpp
@@ +9438,5 @@
>          return true;
>      }
>  
>      MOZ_ASSERT(info().funMaybeLazy());
> +    

Trailing whitespace here and below.

::: js/src/jit/MIR.h
@@ +6730,5 @@
> +    bool congruentTo(const MDefinition* ins) const override {
> +        return congruentIfOperandsEqual(ins);
> +    }
> +    AliasSet getAliasSet() const override {
> +        // An arrow function's lexical |this| value is immutable.

Copy/paste typo.

::: js/src/vm/Interpreter.h
@@ +356,5 @@
>  Lambda(JSContext* cx, HandleFunction fun, HandleObject parent);
>  
>  JSObject*
> +LambdaArrow(JSContext* cx, HandleFunction fun, HandleObject parent, HandleValue thisv,
> +            HandleValue newTargetv);

I think you can just s/newTargetv/newTarget/ in this patch. ("this" is an invalid argument name so we use thisv; newTarget doesn't have that problem.)

::: js/src/vm/Opcodes.h
@@ +1339,5 @@
>       *   Type: Function
>       *   Operands: uint32_t funcIndex
>       *   Stack: this => obj
>       */ \
> +    macro(JSOP_LAMBDA_ARROW, 131, "lambda_arrow", NULL,   5,  2,  1, JOF_OBJECT) \

Don't forget to bump the XDR subtrahend.

::: js/src/vm/Stack.cpp
@@ +45,5 @@
>  
>      JSObject* callee = nullptr;
> +
> +    // newTarget = NullValue is an initial sentinel for "please fill me in from the stack".
> +    // It should never be passed from Ion code.

Not sure why "Ion code" is mentioned in the middle of the interpreter stack code. Remove or clarify?

Comment 35

[Jan de Mooij [:jandem]]

Comment on attachment 8608965 [details] [diff] [review]
Part 8: Support new.target in generator functions.

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

::: js/src/tests/ecma_6/Class/newTargetGenerators.js
@@ +2,5 @@
> +    assertEq(new.target, expected);
> +    assertEq(eval('new.target'), expected);
> +    assertEq((() => new.target)(), expected);
> +    yield (() => new.target);
> +}

Let's also add a test for legacy generators, to make sure we're doing something reasonable, don't assert/crash etc.

::: js/src/vm/Interpreter.cpp
@@ +3868,5 @@
>      gen = &REGS.sp[-2].toObject();
>      // popInlineFrame expects there to be an additional value on the stack to
>      // pop off, so leave "gen" on the stack.
>  
> +    // again, lie to popInlineFrame. Add a "newTarget" to pop later.

Nit: s/again/Again/

Comment 36

[Jan de Mooij [:jandem]]

Comment on attachment 8609066 [details] [diff] [review]
Part 2.3: Address the nits from Jan's first review.

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

Thanks!

Comment 37

[Jason Orendorff [:jorendorff]]

Comment on attachment 8604282 [details] [diff] [review]
Part 5: new.target parser support, reflect support, and tests

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

r=me on the non-JIT parts. I didn't look at any Ion code at all.

::: js/src/frontend/Parser.cpp
@@ +8466,5 @@
> +        return false;
> +
> +    // Let callers decide whether they want to see what we got instead.
> +    // Don't unget the token, since lookahead cannot handle someone calling
> +    // getToken() with a different modifier

I'm sure there's a great reason for doing it this way, but this can't be it. The caller would have to call with TokenStream::Operand again. Same modifier. Right?

::: js/src/frontend/Parser.h
@@ +573,5 @@
>                      InvokedPrediction invoked = PredictUninvoked);
>      Node primaryExpr(TokenKind tt, InvokedPrediction invoked = PredictUninvoked);
>      Node parenExprOrGeneratorComprehension();
>      Node exprInParens();
> +    

Spaces on blank line.

::: js/src/jit/BaselineCompiler.cpp
@@ +2674,5 @@
> +{
> +    MOZ_ASSERT(function());
> +    frame.syncStack(0);
> +
> +

Style nit: no double blank lines inside functions.

@@ +2683,5 @@
> +    masm.moveValue(UndefinedValue(), R0);
> +    masm.jump(&pushResult);
> +
> +    masm.bind(&constructing);
> +    

spaces on blank line (also elsewhere in this file)

@@ +2695,5 @@
> +
> +    masm.branchPtr(Assembler::AboveOrEqual, argvLen, Imm32(function()->nargs()),
> +                   &actualArgsSufficient);
> +    masm.movePtr(ImmWord(function()->nargs()), argvLen);
> +    masm.bind(&actualArgsSufficient);

I'm probably just reading this wrong, but it seems like you should need a masm.jump(&pushResult) just before this .bind()?

::: js/src/jsast.tbl
@@ +38,5 @@
>  ASTDEF(AST_GENERATOR_EXPR,        "GeneratorExpression",            "generatorExpression")
>  ASTDEF(AST_YIELD_EXPR,            "YieldExpression",                "yieldExpression")
>  ASTDEF(AST_LET_EXPR,              "LetExpression",                  "letExpression")
>  ASTDEF(AST_CLASS_EXPR,            "ClassExpression",                "classExpression")
> +ASTDEF(AST_NEWTARGET_EXPR,        "NewTargetExpression",            "newTarget")

Change the string in the right-hand column to "newTargetExpression", please, for consistency.

::: js/src/tests/ecma_6/Class/newTargetArgumentsIntact.js
@@ +5,5 @@
> +
> +function argsWithNewTarget(foo) {
> +    assertEq(arguments.length, argsContent.length);
> +    for (let i = 0; i < arguments.length; i++)
> +        assertEq(arguments[i], argsContent[i]);

Also test that assigning to arguments[arguments.length] doesn't change this.target (this would be stupid, but might as well)

::: js/src/tests/ecma_6/Class/newTargetBound.js
@@ +1,5 @@
> +function boundTarget(expected) {
> +    assertEq(new.target, expected);
> +}
> +
> +let bound = Function.bind(undefined);

This is meant to be boundTarget.bind(undefined), right? I think this test is not calling boundTarget at all...

::: js/src/tests/ecma_6/Class/shell.js
@@ +15,5 @@
> +
> +function assertThrownErrorContains(thunk, substr) {
> +    try {
> +        thunk();
> +        throw new Error("Expected error containing " + substr + ", no exception thrown");

Need to move this `throw` statement outside of the try-block. As written,
    assertThrownErrorContains(_=>0, "unicorn");
passes (it should fail).

::: js/src/tests/js1_8_5/reflect-parse/newTarget.js
@@ +1,3 @@
> +// |reftest| skip-if(!xulRuntime.shell)
> +function testNewTarget() {
> +    

spaces on blank line (several places in this file)

@@ +27,5 @@
> +    
> +    // only new.target is a valid production, no shorn names or other names
> +    assertError("new.", SyntaxError);
> +    assertError("new.foo", SyntaxError);
> +    assertError("new.targe", SyntaxError);

also test with whitespace between the parts ("new\n.\t /* blah */ target")

also test that `obj.new.target` is parsed as `(obj.new).target`

::: js/src/vm/Stack.h
@@ +743,5 @@
> +    Value newTarget() const {
> +        // new.target in eval() NYI.
> +        MOZ_ASSERT(isNonEvalFunctionFrame());
> +        if (isConstructing()) {
> +            unsigned pushedArgs = Max(numFormalArgs(), numActualArgs());

Style nit: #include "jsutil.h" at the top, for Max() (even though it works without it. Include What You Use)

Comment 38

[Eric Faust [:efaust]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> ::: js/src/jit/BaselineJIT.cpp
> @@ +208,5 @@
> > +        if (fp->isEvalFrame()) {
> > +            if (!vals.reserve(2))
> > +                return JitExec_Aborted;
> > +
> > +            vals.infallibleAppend(thisv);
> 
> Hm why do we need to push |this| here now and not before? Is this value used
> anywhere? Do we have to trace it in BaselineFrame::trace?
> 

We pushed it before also. When we set data.maxArgv = thisv.address(), we get the thisv pushed, since data.maxArgc = Max(numActuals (0 for eval), numFormals (0 for eval)) + 1. So this only pushes one new value, we just have to use the vector to hold both now.

> ::: js/src/jit/Lowering.cpp
> @@ +627,2 @@
> >          useBoxAtStart(lir, LCallDirectEvalV::ThisValue, thisValue);
> > +        useBoxAtStart(lir, LCallDirectEvalV::NewTarget, newTargetValue);
> 
> If the profiler is enabled, the frame pointer register is not available.
> That leaves 6 registers on x86; I think this patch needs 7 in the
> CallDirectEvalV case:
> 
> - 2 for LCallDirectEvalV::ThisValue
> - 2 for LCallDirectEvalV::NewTarget
> - 2 for LCallDirectEvalV::Argument
> - 1 for the scope chain
> 
> Please write a shell test that hits this (it should assert in the register
> allocator I think) and fix it. I don't see an obvious fix; maybe we can use
> stack allocation for the scope chain: s/useRegisterAtStart/useAtStart/, or
> get rid of LCallDirectEvalV; not sure when we use that one instead of
> LCallDirectEvalS.
> 

Bah. I was scared to death of this for the BC JSOP_RESUME impl, but totally missed it here. I'll see what I can do about that. Nice catch.
 
> ::: js/src/tests/ecma_6/Class/newTargetEval.js
> @@ +7,5 @@
> > +// new.target is (for now!) invalid inside arrow functions, or eval inside arrow
> > +// functions.
> > +assertThrowsInstanceOf(() => eval('new.target'), SyntaxError);
> > +
> > +function assertNewTarget(expected) { assertEq(eval('new.target'), expected); }
> 
> Please add similar tests for *nested* eval in global/function/arrow scope:
> 
> eval("eval('new.target')");
> eval("eval(\"eval('new.Target')\")")
> 
> We probably also need some tests for indirect eval.
> 

It should not parse inside direct eval. I will add a test to make sure of this, though.

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 8604282 [details] [diff] [review]
Part 5: new.target parser support, reflect support, and tests

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

::: js/src/jit/BaselineCompiler.cpp
@@ +2695,5 @@
> +
> +    masm.branchPtr(Assembler::AboveOrEqual, argvLen, Imm32(function()->nargs()),
> +                   &actualArgsSufficient);
> +    masm.movePtr(ImmWord(function()->nargs()), argvLen);
> +    masm.bind(&actualArgsSufficient);

I was reading it wrong.

Comment 40

[Eric Faust [:efaust]]

(In reply to Eric Faust [:efaust] from comment #38)
> It should not parse inside direct eval. I will add a test to make sure of
> this, though.

Indirect eval, obviously.

Comment 41

[Jason Orendorff [:jorendorff]]

Comment on attachment 8608959 [details] [diff] [review]
Part 6: Support new.target in eval frames.

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

Can the newTarget value be computed in InterpreterFrame::initExecuteFrame, instead of optionally passing it in?

It seems like for frames initialized by that method, the newTarget should be

*   evalInFramePrev.newTarget() if evalInFramePrev is non-null and a function frame;
*   <top of stack iter>.newTarget() if this is direct eval;
*   otherwise, don't care (won't be used)

Clearing r? for now.

Comment 42

[Jason Orendorff [:jorendorff]]

Comment on attachment 8609175 [details] [diff] [review]
Part 7.1: remove some trailing whitespace, and add forgotten test for arrow functions.

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

::: js/src/tests/ecma_6/Class/newTargetArrow.js
@@ +1,2 @@
> +// new.target is valid in any arrow function in a global context.
> +new Function('(() => new.target)()');

I think the code here is correct, but the comment is wrong. The arrow function here is not in a global context: it's nested in the function being created by calling `new Function()`.

    (function () { (() => new.target)() })

@@ +1,5 @@
> +// new.target is valid in any arrow function in a global context.
> +new Function('(() => new.target)()');
> +
> +// It's also good inside eval, but not global eval
> +assertThrowsInstanceOf(() => eval('() => new.target'), SyntaxError);

Also test indirect eval please, preferably from inside a function.

Comment 43

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #41)
> Comment on attachment 8608959 [details] [diff] [review]
> Part 6: Support new.target in eval frames.
> 
> Review of attachment 8608959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can the newTarget value be computed in InterpreterFrame::initExecuteFrame,
> instead of optionally passing it in?
> 
> It seems like for frames initialized by that method, the newTarget should be
> 
> *   evalInFramePrev.newTarget() if evalInFramePrev is non-null and a
> function frame;
> *   <top of stack iter>.newTarget() if this is direct eval;
> *   otherwise, don't care (won't be used)
> 
> Clearing r? for now.

This is almost right, as discussed on IRC. The problem is that if the function is called from Ion, <top of stack iter>.newTarget() is not viable. We cannot recover the newTarget from the snapshot, because it's not in the snapshot! What's worse, as I understand it, we can't put it in the snapshot if we wanted to: We can't have two different snapshot layouts for whether we are constructing or not. At any rate, it's much simpler to just have ion pass the value in to the eval.

Comment 44

[Jason Orendorff [:jorendorff]]

Comment on attachment 8608959 [details] [diff] [review]
Part 6: Support new.target in eval frames.

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

r=me, as long as there's test coverage for recovering new.target in an Ion bailout, somewhere in this stack. (You mentioned on IRC that you're sure there's at least a jit-test for the arg underflow case, though I don't see it in this patch.)

::: js/src/frontend/SharedContext.h
@@ +232,5 @@
>  
> +    enum AllowedSyntax {
> +        AllowSyntax_NewTarget,
> +        AllowSyntax_SuperProperty
> +    };

+1 for enum class

::: js/src/jit-test/tests/debug/Frame-newTargetEval-01.js
@@ +25,5 @@
> +      // assertEq(frame.eval('new.target').value.unsafeDereference(), expected);
> +    }
> +  };
> +
> +  g.eval("" + function f(d) { new g(d, g, 15); });

Template strings?

::: js/src/jit/BaselineFrame.h
@@ +221,5 @@
> +    Value* evalNewTargetAddress() const {
> +        MOZ_ASSERT(isEvalFrame());
> +        return (Value*)(reinterpret_cast<const uint8_t*>(this) +
> +                        BaselineFrame::Size() +
> +                        offsetOfArg(0));

maybe offsetOfEvalNewTarget() instead of offsetOfArg(0) here.

Comment 45

[Jason Orendorff [:jorendorff]]

Comment on attachment 8608962 [details] [diff] [review]
Part 7: Support new.target inside arrow functions

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

::: js/src/vm/Interpreter.cpp
@@ +4032,5 @@
>  END_CASE(JSOP_SUPERBASE)
>  
>  CASE(JSOP_NEWTARGET)
> +    // This should never hit from actual new.target queries, but might happen
> +    // while initalizing arrow functions.

Make the emitter use JSOP_NULL instead of JSOP_NEWTARGET in that case? That way the specialness only has to exist in one place.

::: js/src/vm/Stack.cpp
@@ +45,5 @@
>  
>      JSObject* callee = nullptr;
> +
> +    // newTarget = NullValue is an initial sentinel for "please fill me in from the stack".
> +    // It should never be passed from Ion code.

The clarification might be

    // newTarget = NullValue is an initial sentinel for "please fill me in from the stack".
    // This is always OK except when doing direct eval in an Ion frame.

Comment 46

[Eric Faust [:efaust]]

Attachment: Part 3: Implement proper new.target forwarding semantics for inlined bound functions in ion. v2

I'm not super thrilled about the IonBuilder.cpp change, but tracking it otherwise is a mess.

Comment 47

[Jan de Mooij [:jandem]]

Comment on attachment 8612567 [details] [diff] [review]
Part 3: Implement proper new.target forwarding semantics for inlined bound functions in ion. v2

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

Looks good.

Comment 48

[Eric Faust [:efaust]]

Comment on attachment 8604282 [details] [diff] [review]
Part 5: new.target parser support, reflect support, and tests

Jason didn't want to look (quite reasonably) at the JIT parts. Mind taking a gander?

Comment 49

[Shu-yu Guo [:shu]]

Comment on attachment 8604282 [details] [diff] [review]
Part 5: new.target parser support, reflect support, and tests

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

Codegen parts in Baseline and Ion look fine to me.

::: js/src/jit/BaselineCompiler.cpp
@@ +2676,5 @@
> +    frame.syncStack(0);
> +
> +
> +    // if (!isConstructing()) push(undefined)
> +    Label constructing, pushResult;

I would name pushResult just "done". frame.push(R0) doesn't actually emit any code, just marks R0 as the top of the interpreter stack for syncing.

@@ +2694,5 @@
> +    Label actualArgsSufficient;
> +
> +    masm.branchPtr(Assembler::AboveOrEqual, argvLen, Imm32(function()->nargs()),
> +                   &actualArgsSufficient);
> +    masm.movePtr(ImmWord(function()->nargs()), argvLen);

masm.move32(Imm32(function()->nargs()), argvLen)

@@ +2697,5 @@
> +                   &actualArgsSufficient);
> +    masm.movePtr(ImmWord(function()->nargs()), argvLen);
> +    masm.bind(&actualArgsSufficient);
> +
> +    BaseValueIndex newTarget(BaselineFrameReg, argvLen, BaselineFrame::offsetOfArg(0));

Reads weird, but this is BaselineFrameReg + offsetOfArg(0) + (argvLen * sizeof(Value)), right?

@@ +2699,5 @@
> +    masm.bind(&actualArgsSufficient);
> +
> +    BaseValueIndex newTarget(BaselineFrameReg, argvLen, BaselineFrame::offsetOfArg(0));
> +    masm.loadValue(newTarget, R0);
> +    

Trailing whitespace.

::: js/src/jit/CodeGenerator.cpp
@@ +9994,5 @@
> +    masm.moveValue(UndefinedValue(), output);
> +    masm.jump(&done);
> +
> +    masm.bind(&constructing);
> +    

Trailing whitespace.

@@ +9998,5 @@
> +    
> +    // else output = argv[Max(numActualArgs, numFormalArgs)]
> +    Register argvLen = output.scratchReg();
> +
> +    Address actualArgs(masm.getStackPointer(), frameSize() + JitFrameLayout::offsetOfNumActualArgs());

Maybe actualArgsPtr?

@@ +10006,5 @@
> +
> +    size_t numFormalArgs = ins->mirRaw()->block()->info().funMaybeLazy()->nargs();
> +    masm.branchPtr(Assembler::AboveOrEqual, argvLen, Imm32(numFormalArgs),
> +                   &actualArgsSufficient);
> +    masm.movePtr(ImmWord(numFormalArgs), argvLen);

move32, Imm32

@@ +10011,5 @@
> +    masm.bind(&actualArgsSufficient);
> +
> +    BaseValueIndex newTarget(masm.getStackPointer(), argvLen, frameSize() + JitFrameLayout::offsetOfActualArgs());
> +    masm.loadValue(newTarget, output);
> +    

Trailing whitespace.

::: js/src/jit/MIR.h
@@ +11904,5 @@
>  };
>  
> +class MNewTarget : public MNullaryInstruction
> +{
> +    MNewTarget() : MNullaryInstruction() {

Style nit:

  MNewTarget()
    : MNullaryInstruction()
  {

Comment 50

[Eric Faust [:efaust]]

Attachment: Part 6.1: Remove LCallDirectEvalV, as it runs into register pressure with new.target

(In reply to Jan de Mooij [:jandem] from comment #33)
> ::: js/src/jit/Lowering.cpp
> @@ +627,2 @@
> >          useBoxAtStart(lir, LCallDirectEvalV::ThisValue, thisValue);
> > +        useBoxAtStart(lir, LCallDirectEvalV::NewTarget, newTargetValue);
> 
> If the profiler is enabled, the frame pointer register is not available.
> That leaves 6 registers on x86; I think this patch needs 7 in the
> CallDirectEvalV case:
> 
> - 2 for LCallDirectEvalV::ThisValue
> - 2 for LCallDirectEvalV::NewTarget
> - 2 for LCallDirectEvalV::Argument
> - 1 for the scope chain
> 
> Please write a shell test that hits this (it should assert in the register
> allocator I think) and fix it. I don't see an obvious fix; maybe we can use
> stack allocation for the scope chain: s/useRegisterAtStart/useAtStart/, or
> get rid of LCallDirectEvalV; not sure when we use that one instead of
> LCallDirectEvalS.
> 

We use it if we can't tell whether the thing is for sure not a string, or for sure a string.

I think we can safely remove it. I tried useAtStart, and the problem is that the resulting operand is a relative offset from the stack pointer, which keeps moving on us as we push args for the callVM. Instead of doing something brittle, like changing the offset to account for the pushes (though maybe having the macroassembler keep track of the number of words pushed to count how much to swizzle makes it "slightly less ugly"), or reordering the args (so that the relative operand was the first thing pushed), I opted to just remove LCallDirectEvalV.

Comment 51

[Jan de Mooij [:jandem]]

Comment on attachment 8613182 [details] [diff] [review]
Part 6.1: Remove LCallDirectEvalV, as it runs into register pressure with new.target

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

::: js/src/jit/TypePolicy.cpp
@@ +1142,5 @@
>      _(DoublePolicy<0>)                                                  \
>      _(FloatingPointPolicy<0>)                                           \
>      _(IntPolicy<0>)                                                     \
>      _(IntPolicy<1>)                                                     \
> +    _(Mix3Policy<ObjectPolicy<0>, StringPolicy<1>, BoxPolicy<2> >) \

Nit: add some spaces so the '\' lines up.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9a06c26be7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbf3e53cce6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b87d4ed5d30
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9d645f55b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/027bba006fbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a10ac99656
https://hg.mozilla.org/integration/mozilla-inbound/rev/3796a4c827e4

Comment 53

[Pulsebot]

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

Comment 54

[Pulsebot]

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

Comment 55

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out between
https://hg.mozilla.org/integration/mozilla-inbound/rev/d245fc3d7b54
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6e1e804fab
and
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f81ddc32a9

for assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10336676&repo=mozilla-inbound in various test failures

Comment 56

[Hannes Verschore [:h4writer]]

Just a precaution. But this patch also introduced a regression. Upon backout the numbers went back to normal.
Can you also take a look to make sure that before pushing again this regression is gone?

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 64-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- octane: Richards: -2.78% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=baa684fab4f5&tochange=c0e7bdb0da7b

More details: http://arewefastyet.com/regressions/#/regression/1713575

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2901436c9047
https://hg.mozilla.org/integration/mozilla-inbound/rev/d038c5da19b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f94d0d72a21
https://hg.mozilla.org/integration/mozilla-inbound/rev/6031f4ce0f9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/24403e652d6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f963fbdc918
https://hg.mozilla.org/integration/mozilla-inbound/rev/213b09228bdf

Comment 58

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10391744&repo=mozilla-inbound

Comment 59

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05411d0a95ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/b003dbf1e253
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7268073cd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a837cfd8ee1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/28087e3f22f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/23fcf07dcd9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2064e2c511

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00fafdaf5c1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/02a02fcab9eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fff120d6bef
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d30f8210cd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d159eb02b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7b5bd0505d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22779079708

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00fafdaf5c1e
https://hg.mozilla.org/mozilla-central/rev/02a02fcab9eb
https://hg.mozilla.org/mozilla-central/rev/7fff120d6bef
https://hg.mozilla.org/mozilla-central/rev/4d30f8210cd6
https://hg.mozilla.org/mozilla-central/rev/d1d159eb02b1
https://hg.mozilla.org/mozilla-central/rev/5f7b5bd0505d
https://hg.mozilla.org/mozilla-central/rev/d22779079708

Comment 62

[Eric Faust [:efaust]]

This landed. Clearing ni?

Comment 63

[Florian Scholz (Open Web Docs)]

Mentioned on: 
https://developer.mozilla.org/en-US/Firefox/Releases/41#JavaScript
Reference page:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target

Review appreciated. I bet someone can explain this better :)