[es6] Perform the IsConstructor test in GetSuperConstructor.

[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a
new bytecode GetSuperConstructor is introduced.

BUG=v8:5336
Review-Url: https://codereview.chromium.org/2504553003
Cr-Commit-Position: refs/heads/master@{#41788}
Committed: https://chromium.googlesource.com/v8/v8/+/815f91c0ed7bd602246feeb0edcdf01633dbd914

[Henrique Ferreiro, 2016-11-15 09:54:26]

henrique.ferreiro@gmail.com changed reviewers:
+ caitp@igalia.com

[Henrique Ferreiro, 2016-11-21 13:29:37]

henrique.ferreiro@gmail.com changed reviewers:
+ petermarshall@chromium.org

[Henrique Ferreiro, 2016-11-21 13:29:37]

I converted GetSuperConstructor to a bytecode, as discussed in the bug report. I
did by looking at how other bytecodes were implemented, so there are many things
wrong. I would appreciate a brief look at what is missing or wrong, or a pointer
to a document on how to do this.

[caitp, 2016-11-21 13:52:54]

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:7821: Node* map = LoadMap(active_function);
I would add a CSA_ASSERT() to ensure that this is a HeapObject

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:7825: Label not_constructor(this);
You want this label to be deferred.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:350:
NodeProperties::ReplaceContextInput(node, jsgraph()->NoContextConstant());
I would just replace this whole block with `ReplaceWithRuntimeCall(node,
Runtime::kGetSuperConstructor);`,  for now (unless we already have a codestub
for this).

I'm not sure exactly how the pipeline works, but I feel in the monomorphic case
we should be able to eliminate the IsConstructor() check and the CallRuntime() /
Throw(), but I'm actually not positive how to do that with js operators.

@bmeurer?

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/opcodes.h
File src/compiler/opcodes.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/opcodes.h#...
src/compiler/opcodes.h:141: V(JSGetSuperConstructor)
Do we need this to be a JS operator instead of a simplified operator? (Do we
need the Context input, or can we get that during inlining of the class 
constructor?)

[caitp, 2016-11-22 23:30:34]

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:1025: Node* object =
assembler->Parameter(Descriptor::kObject);
if `object` is an Smi, GetSuperConstructor() will either crash or do something
weird/undefined.

[caitp, 2016-11-22 23:42:04]

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:1021: void
Builtins::Generate_GetSuperConstructor(CodeStubAssembler* assembler) {
It looks like this should take a compiler::CodeAssemblerState* parameter, not a
CodeStubAssembler. Based on my read of other TFS stubs, you need to create the
assembler yourself, eg `CodeStubAssembler assembler(state);`

[Benedikt Meurer, 2016-11-23 04:30:34]

Description was changed from

==========
Perform the IsConstructor test in GetSuperConstructor

This is so that a NotConstructor error is thrown before evaluating the
arguments to the super constructor.
BUG=v8:5336
==========

to

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotConstructor error is thrown before evaluating the
arguments to the super constructor.

BUG=v8:5336
==========

[Benedikt Meurer, 2016-11-23 04:50:59]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[Benedikt Meurer, 2016-11-23 04:50:59]

bmeurer@chromium.org changed required reviewers:
+ rmcilroy@chromium.org

[Benedikt Meurer, 2016-11-23 04:51:01]

Very nice. First round of comments.

Adding Ross for the interpreter review (Ross, please check my comment on the
GetSuperConstructor bytecode).

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:1021: void
Builtins::Generate_GetSuperConstructor(CodeStubAssembler* assembler) {
Jep, you'll probably hit this once you rebase.

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:7821: Node* map = LoadMap(active_function);
Yes, please.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:350:
NodeProperties::ReplaceContextInput(node, jsgraph()->NoContextConstant());
This should use the GetSuperConstructor builtin:

CallDescriptor::Flags flags = FrameStateFlagFor(node);
Callable callable = CodeFactory::GetSuperConstructor(isolate());
ReplaceWithStubCall(node, callable, flags);

We will eventually turn the GetSuperConstructor bytecode into an IC and collect
feedback that will allow us to eliminate and constant-fold it completely most of
the time. But the generic case still matters.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:457: V(GetSuperConstructor, Operator::kNoProperties,
1, 1)
This should be marked as Operator::kNoWrite.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/opcodes.h
File src/compiler/opcodes.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/opcodes.h#...
src/compiler/opcodes.h:141: V(JSGetSuperConstructor)
It can raise exceptions, thus it needs a proper context. Also the contract is
currently, that everything that can potentially throw or has some other
JavaScriptish semantics, has to be a JS operator. (it's not the best contract, I
know).

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:1213: return Type::Function();
This is probably not correct, as this can return any value v for which
IsConstructor(v) returns true, i.e. including proxies, bound functions and
special API objects. So the type should probably by Type::Receiver here.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/verifier.c...
src/compiler/verifier.cc:615: // Type is Function.
Nit: Type is Receiver.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/verifier.c...
src/compiler/verifier.cc:616: CheckTypeIs(node, Type::Function());
Nit: Type::Receiver

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2489: builder()->GetSuperConstructor();
We should probably pass an output register to this bytecode, because we will
always store the result to some register anyways, so we could save the Star
bytecode. Ross?

https://codereview.chromium.org/2504553003/diff/20001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:408: return
Runtime_ThrowConstructedNonConstructable(1, &prototype, isolate);
Please don't call other runtime functions this way. Just do the
THROW_NEW_ERROR_RETURN_FAILURE here.
Also this deserves a useful error message IMHO, i.e. something like "super
constructor <bar> of <foo> is not a constructor".

[rmcilroy, 2016-11-23 21:55:38]

Interpreter looks good overall. Couple of suggestions.

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:209:
Output(Bytecode::kGetSuperConstructor);
This looks like it's on a really old revision of V8. When you rebase you will
need to make this OutputGetSuperConstructor().

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:237: BytecodeArrayBuilder&
GetSuperConstructor();
Please move this to it's own section and describe what it does.

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2489: builder()->GetSuperConstructor();
On 2016/11/23 04:51:00, Benedikt Meurer wrote:
> We should probably pass an output register to this bytecode, because we will
> always store the result to some register anyways, so we could save the Star
> bytecode. Ross?

Yup sounds good, just add a kRegOut operand and pass the register as an operand.

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:222: case Bytecode::kGetSuperConstructor:
Remove this if you add the register as an operand.

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:144: V(GetSuperConstructor,
AccumulatorUse::kReadWrite)                           \
This isn't really a unary op (in the sense of JS unary ops), please just move it
to it's own section.

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1592: void
Interpreter::DoGetSuperConstructor(InterpreterAssembler* assembler) {
Please add a comment like those above other bytecode handlers.

[Henrique Ferreiro, 2016-12-01 12:22:23]

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:1025: Node* object =
assembler->Parameter(Descriptor::kObject);
On 2016/11/22 at 23:30:34, caitp wrote:
> if `object` is an Smi, GetSuperConstructor() will either crash or do something
weird/undefined.

Shouldn't I check that in CodeStubAssembler::GetSuperConstructor instead of
using a CSA_ASSERT?

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins.h...
src/builtins/builtins.h:180: TFS(GetSuperConstructor, BUILTIN, kNoExtraICState,
TypeConversion)          \
TypeConversion does not seem to apply here, does it?

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:7821: Node* map = LoadMap(active_function);
On 2016/11/21 at 13:52:53, caitp wrote:
> I would add a CSA_ASSERT() to ensure that this is a HeapObject

Similar code checks for a Smi and a HeapNumber, is the latter neccessary? I
would assume a heap number also has a map.

https://codereview.chromium.org/2504553003/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:7825: Label not_constructor(this);
On 2016/11/21 at 13:52:53, caitp wrote:
> You want this label to be deferred.

Would you mind explaining what is that for?

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:457: V(GetSuperConstructor, Operator::kNoProperties,
1, 1)
On 2016/11/23 at 04:51:00, Benedikt Meurer wrote:
> This should be marked as Operator::kNoWrite.

Would you mind explaining what would be the difference with ToName and similar
operators which use kNoProperties? I would assume they work in the same way.

[Benedikt Meurer, 2016-12-01 13:11:51]

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:1025: Node* object =
assembler->Parameter(Descriptor::kObject);
Yep.

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2504553003/diff/20001/src/builtins/builtins.h...
src/builtins/builtins.h:180: TFS(GetSuperConstructor, BUILTIN, kNoExtraICState,
TypeConversion)          \
On 2016/12/01 12:22:22, hferreiro wrote:
> TypeConversion does not seem to apply here, does it?

The descriptor is probably OK technically, but confusing.

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/2504553003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:457: V(GetSuperConstructor, Operator::kNoProperties,
1, 1)
ToName can call into arbitrary JavaScript (via ToPrimitive conversions). But
GetSuperConstructor will never execute arbitrary JavaScript, so it has no
observable side effects (which is expressed by kNoWrite, yeah I know the name
sucks).

[Henrique Ferreiro, 2016-12-08 13:18:01]

I think I have addressed all comments. Can/should I remove the runtime function?
I noticed it is called from the parser, though.

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1617: Environment::kAttachFrameState);
Is attaching the frame state correct? Is it related to the operator throwing
exceptions?

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (left):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:154: case Runtime::kGetSuperConstructor:
I assumed I had to remove this.

[Benedikt Meurer, 2016-12-08 18:42:24]

Nice, thanks for the hard work! Looking good.

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:5304: GotoUnless(IsConstructorMap(callable_map),
&return_runtime);
This is not correct, as the original code also tests for non-instance
prototypes. Please don't change this code *(it's also unrelated to your CL).

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1617: Environment::kAttachFrameState);
Yes and yes :-)

Since this operation can throw an exception we need to be able to construct a
proper stack trace, which is done by looking at the deoptimization information.

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (left):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:154: case Runtime::kGetSuperConstructor:
Just leave 'em here as long as the parser still uses it. We remove it when we
drop the runtime function completely.

https://codereview.chromium.org/2504553003/diff/40001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:61: namespace {
Nit: empty line after { and before } below.

https://codereview.chromium.org/2504553003/diff/40001/test/test262/test262.st...
File test/test262/test262.status (left):

https://codereview.chromium.org/2504553003/diff/40001/test/test262/test262.st...
test/test262/test262.status:428:
'language/expressions/super/call-proto-not-ctor': [FAIL],
W00t!

[Henrique Ferreiro, 2016-12-09 13:24:28]

I had to cope with the constructor passed to the runtime function being an
Oddball and potentially other things than a JSFunction. Am I getting the correct
string in each case?

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:5304: GotoUnless(IsConstructorMap(callable_map),
&return_runtime);
On 2016/12/08 18:42:24, Benedikt Meurer wrote:
> This is not correct, as the original code also tests for non-instance
> prototypes. Please don't change this code *(it's also unrelated to your CL).

Sorry, I should have double-checked. Anyway, is it not ok to do small
refactorings when introducing helper functions or so?

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (left):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:154: case Runtime::kGetSuperConstructor:
On 2016/12/08 18:42:24, Benedikt Meurer wrote:
> Just leave 'em here as long as the parser still uses it. We remove it when we
> drop the runtime function completely.

The runtime function throws an exception now, so it needs a FrameState, right?

[Benedikt Meurer, 2016-12-09 18:30:28]

Thanks a lot. Next round of comments.

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:5304: GotoUnless(IsConstructorMap(callable_map),
&return_runtime);
Better do a separate CL for those. Unrelated changes are confusing, esp. when
things go wrong later (i.e. Chrome is crashing) and we cannot figure out why
something was changed.

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (left):

https://codereview.chromium.org/2504553003/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:154: case Runtime::kGetSuperConstructor:
You're absolutely right, thanks for pointing this out!

https://codereview.chromium.org/2504553003/diff/60001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:7836: GotoIf(TaggedIsSmi(active_function),
&is_not_object);
This looks wrong. You first load the map, thereby assuming it's a HeapObject,
then you do a Smi check later.

The active_function is always a JSFunction, you should CSA_ASSERT that. If it's
ever anything but a JSFunction, then you're in trouble. The active_function is
always passed the this_function_var, which refers to the JSFunction in the
activation record.

https://codereview.chromium.org/2504553003/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:7844: Bind(&is_not_object);
You don't need this label and the message below. The active_function is always a
JSFunction, the only error condition is that the prototype of the
active_function is not a constructor.

https://codereview.chromium.org/2504553003/diff/60001/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/2504553003/diff/60001/src/compiler/verifier.c...
src/compiler/verifier.cc:638: case IrOpcode::kJSGetSuperConstructor:
Check that the value input is Type::Function().

https://codereview.chromium.org/2504553003/diff/60001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2504553003/diff/60001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:69: } else if (constructor->IsOddball()) {
As far as I can tell, this should always be null if it's an Oddball, which means
there's no prototype at all.

https://codereview.chromium.org/2504553003/diff/60001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:81: }  // anonymous namespace
Nit: just namespace here.

[Henrique Ferreiro, 2016-12-12 12:12:28]

Uploaded a new revision. Added code to AllBytecodesGenerated unittest that was
failing.

[Benedikt Meurer, 2016-12-12 18:18:19]

Awesome, thank you for iterating this! LGTM once comments are addressed.

https://codereview.chromium.org/2504553003/diff/80001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504553003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:7851: result.Bind(UndefinedConstant());  // Never
reached.
Nit: you could just bind the result of the runtime call, that'd generate shorter
code, i.e. save the ldr rax, [roots+UndefinedOffset]. But it's not really
important.

https://codereview.chromium.org/2504553003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2504553003/diff/80001/src/compiler/typer.cc#n...
src/compiler/typer.cc:1259: return Type::Receiver();
You can say Type::Callable() here which I introduced recently, as this will
always return a constructor, and any constructor is callable (we don't have a
notion of callable in the type system, and I don't think it makes sense now, so
go for callable).

https://codereview.chromium.org/2504553003/diff/80001/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/2504553003/diff/80001/src/compiler/verifier.c...
src/compiler/verifier.cc:641: CheckTypeIs(node, Type::Receiver());
Same: Use Type::Callable here.

[rmcilroy, 2016-12-12 19:43:19]

Interpreter LGTM with a nit.

https://codereview.chromium.org/2504553003/diff/80001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2504553003/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1700: // GetSuperConstructor
nit - add operand "<reg>" and mention in the description that the result gets
stored in this register.

[Henrique Ferreiro, 2016-12-13 11:10:27]

Description was changed from

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotConstructor error is thrown before evaluating the
arguments to the super constructor.

BUG=v8:5336
==========

to

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a new
bytecode GetSuperConstructor is introduced.

BUG=v8:5336
==========

[Henrique Ferreiro, 2016-12-13 11:12:08]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 11:12:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 11:13:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 11:13:49]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17720)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17786)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17695)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30499)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19429)

[Henrique Ferreiro, 2016-12-13 12:17:11]

Description was changed from

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a new
bytecode GetSuperConstructor is introduced.

BUG=v8:5336
==========

to

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a
new bytecode GetSuperConstructor is introduced.

BUG=v8:5336
==========

[Henrique Ferreiro, 2016-12-13 12:17:20]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 12:17:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 12:21:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 12:21:20]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17724)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19304)

[Henrique Ferreiro, 2016-12-13 12:47:49]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 12:47:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 13:02:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 13:02:29]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Henrique Ferreiro, 2016-12-14 12:39:04]

I'm getting this from mjsunit/es6/classes-lazy-parsing:

# Fatal error in ../../src/compiler/verifier.cc, line 84
# TypeError: node #16:JSGetSuperConstructor(input @0 =
JSLoadContext:JSLoadContext) type Any is not Function

Any hint?

Additionally, webkit/class-syntax-super is failing when using anonymous classes:

FAIL new (class extends null { constructor() { super() } }) should throw
TypeError: super is not a constructor. Threw exception TypeError: Super
constructor  of  is not a constructor

I guess I'll need a new error message for the case where the class is nameless.
Would it be correct to use 'null' for the case of the constructor name returning
an empty string?

Lastly, isn't there a trybot running the turbofan_opt variant? I am getting this
error when running test262/language/expressions/super/call-proto-not-ctor:

# Fatal error in ../../src/frames.cc, line 1188
# Missing deoptimization information for OptimizedFrame::Summarize.

[Benedikt Meurer, 2016-12-14 18:27:16]

On 2016/12/14 12:39:04, hferreiro wrote:
> I'm getting this from mjsunit/es6/classes-lazy-parsing:
> 
> # Fatal error in ../../src/compiler/verifier.cc, line 84
> # TypeError: node #16:JSGetSuperConstructor(input @0 =
> JSLoadContext:JSLoadContext) type Any is not Function

Mhm, this_function can be context-allocated it seems, so you need to check for
Type::Any on the input in the verifier instead.
Maybe put a comment there that we don't check for Type::Function because
this_function can be context-allocated.

> Additionally, webkit/class-syntax-super is failing when using anonymous
classes:
> 
> FAIL new (class extends null { constructor() { super() } }) should throw
> TypeError: super is not a constructor. Threw exception TypeError: Super
> constructor  of  is not a constructor
> 
> I guess I'll need a new error message for the case where the class is
nameless.
> Would it be correct to use 'null' for the case of the constructor name
returning
> an empty string?

Yes, and yes.

> Lastly, isn't there a trybot running the turbofan_opt variant? I am getting
this
> error when running test262/language/expressions/super/call-proto-not-ctor:
> 
> # Fatal error in ../../src/frames.cc, line 1188
> # Missing deoptimization information for OptimizedFrame::Summarize.

You need to return true for JSGetSuperConstructor from
OperatorProperties::HasFrameStateInput in operator-properties.cc (somehow we
missed that).

[Henrique Ferreiro, 2016-12-16 10:47:58]

https://codereview.chromium.org/2504553003/diff/160001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2504553003/diff/160001/src/messages.h#newcode345
src/messages.h:345: T(NotSuperConstructor, "Super constructor % of % is not a
constructor.")     \
I am seeing mixed use of the trailing dot. Which one is correct?

[Henrique Ferreiro, 2016-12-16 10:48:10]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 10:48:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2016-12-16 10:48:38]

https://codereview.chromium.org/2504553003/diff/160001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2504553003/diff/160001/src/messages.h#newcode345
src/messages.h:345: T(NotSuperConstructor, "Super constructor % of % is not a
constructor.")     \
On 2016/12/16 10:47:57, hferreiro wrote:
> I am seeing mixed use of the trailing dot. Which one is correct?

Pick one, you decide :-)

[commit-bot: I haz the power, 2016-12-16 10:49:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 10:49:45]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17944)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18007)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30776)

[Henrique Ferreiro, 2016-12-16 10:54:19]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 10:54:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 10:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 10:55:56]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17945)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17914)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30777)

[Henrique Ferreiro, 2016-12-16 11:47:25]

https://codereview.chromium.org/2504553003/diff/220001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2504553003/diff/220001/src/builtins/builtins....
src/builtins/builtins.h:714: TFJ(TypedArrayPrototypeValues, 0)
This change was made by git cl format.

[Henrique Ferreiro, 2016-12-16 11:47:37]

The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 11:47:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 12:15:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 12:15:17]

Dry run: This issue passed the CQ dry run.

[Henrique Ferreiro, 2016-12-16 13:22:40]

The CQ bit was checked by henrique.ferreiro@gmail.com

[Henrique Ferreiro, 2016-12-16 13:22:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2504553003/#ps220001
(title: "rebase and git cl format")

[commit-bot: I haz the power, 2016-12-16 13:22:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 13:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 13:26:36]

Failed to apply patch for src/builtins/builtins.h:
While running git apply --index -p1;
  error: patch failed: src/builtins/builtins.h:179
  error: src/builtins/builtins.h: patch does not apply

Patch:       src/builtins/builtins.h
Index: src/builtins/builtins.h
diff --git a/src/builtins/builtins.h b/src/builtins/builtins.h
index
40cbfef9df44b49804a402020553ca0963aba5ff..3de825303cd4e802bc9f6bd5fc0c4d3d70c2c86a
100644
--- a/src/builtins/builtins.h
+++ b/src/builtins/builtins.h
@@ -179,6 +179,7 @@ namespace internal {
   TFS(ToInteger, BUILTIN, kNoExtraICState, TypeConversion)                    \
   TFS(ToLength, BUILTIN, kNoExtraICState, TypeConversion)                     \
   TFS(Typeof, BUILTIN, kNoExtraICState, Typeof)                               \
+  TFS(GetSuperConstructor, BUILTIN, kNoExtraICState, TypeConversion)          \
                                                                               \
   /* Handlers */                                                              \
   TFS(KeyedLoadIC_Megamorphic_TF, KEYED_LOAD_IC, kNoExtraICState,             \
@@ -710,7 +711,7 @@ namespace internal {
   /* ES6 #sec-%typedarray%.prototype.keys */                                  \
   TFJ(TypedArrayPrototypeKeys, 0)                                             \
   /* ES6 #sec-%typedarray%.prototype.values */                                \
-  TFJ(TypedArrayPrototypeValues, 0)                                           \
+  TFJ(TypedArrayPrototypeValues, 0)
 
 #define IGNORE_BUILTIN(...)

[Henrique Ferreiro, 2016-12-19 09:41:26]

The CQ bit was checked by henrique.ferreiro@gmail.com

[Henrique Ferreiro, 2016-12-19 09:41:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2504553003/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-19 09:41:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 10:12:24]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482140486645240,
"parent_rev": "b695c38842a05808106d3246f0c034bb0d2ebbcc", "commit_rev":
"815f91c0ed7bd602246feeb0edcdf01633dbd914"}

[commit-bot: I haz the power, 2016-12-19 10:12:28]

Description was changed from

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a
new bytecode GetSuperConstructor is introduced.

BUG=v8:5336
==========

to

==========
[es6] Perform the IsConstructor test in GetSuperConstructor.

This is so that a NotSuperConstructor error is thrown before evaluating the
arguments to the super constructor. Besides updating the runtime function, a
new bytecode GetSuperConstructor is introduced.

BUG=v8:5336

Review-Url: https://codereview.chromium.org/2504553003
Cr-Commit-Position: refs/heads/master@{#41788}
Committed:
https://chromium.googlesource.com/v8/v8/+/815f91c0ed7bd602246feeb0edcdf01633d...
==========

[commit-bot: I haz the power, 2016-12-19 10:12:29]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/v8/v8/+/815f91c0ed7bd602246feeb0edcdf01633d...