Closed Bug 1141865 (new.target) Opened 9 years ago Closed 9 years ago

Implement ES6 NewTarget

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: efaust, Assigned: efaust)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(13 files, 5 obsolete files)

11.48 KB, patch
jandem
: review+
Details | Diff | Splinter Review
99.48 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.69 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
48.43 KB, patch
jorendorff
: review+
shu
: review+
Details | Diff | Splinter Review
2.01 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.98 KB, patch
shu
: review+
Details | Diff | Splinter Review
67.79 KB, patch
jandem
: review+
jorendorff
: review+
Details | Diff | Splinter Review
34.58 KB, patch
jandem
: review+
jorendorff
: review+
Details | Diff | Splinter Review
18.99 KB, patch
jandem
: review+
jorendorff
: review+
Details | Diff | Splinter Review
10.74 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.85 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.99 KB, patch
jandem
: review+
Details | Diff | Splinter Review
11.85 KB, patch
jandem
: review+
Details | Diff | Splinter Review
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.
This is not necessarily exclusive to classes, right?
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
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.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8592005 - Flags: review?(jdemooij)
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.
Attachment #8592006 - Flags: feedback?(jorendorff)
Attachment #8592006 - Flags: feedback?(jdemooij)
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.
Attachment #8592005 - Flags: review?(jdemooij) → review+
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".
Attachment #8592006 - Flags: feedback?(jdemooij) → feedback+
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.
Attached patch WIP: base (part 1 of brokenness) (obsolete) — Splinter Review
Uploading to get some debugging help
Attached patch WIP: breaking (brokenness pt 2) (obsolete) — Splinter Review
Attachment #8592006 - Attachment is obsolete: true
Attachment #8601319 - Attachment is obsolete: true
Attachment #8601320 - Attachment is obsolete: true
Attachment #8592006 - Flags: feedback?(jorendorff)
Attachment #8604279 - Flags: review?(jorendorff)
Attachment #8604279 - Flags: review?(jdemooij)
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.
(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.
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.
Attachment #8604934 - Flags: review?(shu)
Clean up the test a little ;)
Attachment #8604934 - Attachment is obsolete: true
Attachment #8604934 - Flags: review?(shu)
Attachment #8604943 - Flags: review?(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.
Attachment #8604943 - Flags: review?(shu) → review+
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
Attachment #8604279 - Flags: review?(jdemooij)
Attachment #8604496 - Flags: review?(jdemooij) → review+
(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.
See comment 21
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
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.
Attachment #8608959 - Flags: review?(jorendorff)
Attachment #8608959 - Flags: review?(jdemooij)
Attachment #8608962 - Flags: review?(jorendorff)
Attachment #8608962 - Flags: review?(jdemooij)
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.
Attachment #8608965 - Flags: review?(jorendorff)
Attachment #8608965 - Flags: review?(jdemooij)
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.
Attachment #8609066 - Flags: review?(jdemooij)
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.
Attachment #8604279 - Flags: review?(jorendorff) → review+
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.
Attachment #8604281 - Flags: review?(jorendorff) → review+
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?
Attachment #8604280 - Flags: review?(jdemooij)
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.
Attachment #8608959 - Flags: review?(jdemooij) → review+
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?
Attachment #8608962 - Flags: review?(jdemooij) → review+
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/
Attachment #8608965 - Flags: review?(jdemooij) → review+
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!
Attachment #8609066 - Flags: review?(jdemooij) → review+
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)
Attachment #8604282 - Flags: review?(jorendorff) → review+
(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 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.
(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 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.
Attachment #8608959 - Flags: review?(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.
Attachment #8609175 - Flags: review?(jorendorff) → review+
(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 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.
Attachment #8608959 - Flags: review+
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.
Attachment #8608962 - Flags: review?(jorendorff) → review+
Attachment #8608965 - Flags: review?(jorendorff) → review+
I'm not super thrilled about the IonBuilder.cpp change, but tracking it otherwise is a mess.
Attachment #8604280 - Attachment is obsolete: true
Attachment #8612567 - Flags: review?(jdemooij)
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.
Attachment #8612567 - Flags: review?(jdemooij) → review+
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?
Attachment #8604282 - Flags: review?(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()
  {
Attachment #8604282 - Flags: review?(shu) → review+
(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.
Attachment #8613182 - Flags: review?(shu)
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.
Attachment #8613182 - Flags: review?(shu) → review+
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
Depends on: 1171597
This landed. Clearing ni?
Flags: needinfo?(efaustbmo)
Alias: new.target
Depends on: 1199057
Depends on: 1207571
You need to log in before you can comment on or make changes to this bug.