Install the 'name' property in classes at runtime

Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure 'name' is added last, to be standards-compliant.

BUG=v8:4199
Committed: https://crrev.com/afd5ff553bbc8abdee37661d6a88af253673aa7c
Cr-Commit-Position: refs/heads/master@{#41546}

[Henrique Ferreiro, 2016-10-17 11:27:58]

Description was changed from

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

Implementations in fullcodegen are missing for most architectures pending
review.

BUG=v8:4199
==========

to

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

Implementations in fullcodegen are missing for most architectures pending
review.

BUG=v8:4199
==========

[Henrique Ferreiro, 2016-10-17 11:41:10]

henrique.ferreiro@gmail.com changed reviewers:
+ adamk@chromium.org, bmeurer@chromium.org, caitp@igalia.com,
rmcilroy@chromium.org

[Henrique Ferreiro, 2016-10-17 11:41:11]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:688:
empty_string_(info->isolate()->factory()->empty_string()) {}
Leftover from a preview version

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
Should I use a runtime function to check for this?

[rmcilroy, 2016-10-24 14:22:20]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1619: void
BytecodeGenerator::VisitClassLiteralNameProperty(ClassLiteral* expr,
Nit - BuildClassLiteralNameProperty

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/10/17 11:41:11, hferreiro wrote:
> Should I use a runtime function to check for this?

You could either do this in the runtime function I suggest adding below, or have
the check not dereference a handle (e.g., check raw_name() or add some field on
the AST node which states whether the name is empty or not).

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1653:
.CallRuntime(Runtime::kDefineDataPropertyInLiteral, receiver, 5);
Rather than creating a load of bytecode for this (which calls into the runtime
anyway), how about just creating a runtime function which does what you want
(e.g., checks if name is undefined or already defined on the property, and if
not, sets it on the class object).

[caitp, 2016-10-24 15:56:10]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/10/17 11:41:11, hferreiro wrote:
> Should I use a runtime function to check for this?

It looks like expr->class_variable_proxy() returns nullptr for anonymous
classes, so I think you could check that instead of checking the constructor
name.

[Henrique Ferreiro, 2016-11-08 22:28:58]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/10/24 15:56:10, caitp wrote:
> On 2016/10/17 11:41:11, hferreiro wrote:
> > Should I use a runtime function to check for this?
> 
> It looks like expr->class_variable_proxy() returns nullptr for anonymous
> classes, so I think you could check that instead of checking the constructor
> name.

I have moved everything to a runtime function. Is there any overhead in calling
them? Should I first check class_variable_proxy() in case I can omit the call?

[caitp, 2016-11-08 22:59:40]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/11/08 22:28:58, hferreiro wrote:
> On 2016/10/24 15:56:10, caitp wrote:
> > On 2016/10/17 11:41:11, hferreiro wrote:
> > > Should I use a runtime function to check for this?
> > 
> > It looks like expr->class_variable_proxy() returns nullptr for anonymous
> > classes, so I think you could check that instead of checking the constructor
> > name.
> 
> I have moved everything to a runtime function. Is there any overhead in
calling
> them? Should I first check class_variable_proxy() in case I can omit the call?

I'll defer to Ignition experts to answer that --- I assume it's not as bad as it
is to call a runtime function from JIT code, but I don't know.

[rmcilroy, 2016-11-10 12:37:44]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/11/08 22:59:40, caitp wrote:
> On 2016/11/08 22:28:58, hferreiro wrote:
> > On 2016/10/24 15:56:10, caitp wrote:
> > > On 2016/10/17 11:41:11, hferreiro wrote:
> > > > Should I use a runtime function to check for this?
> > > 
> > > It looks like expr->class_variable_proxy() returns nullptr for anonymous
> > > classes, so I think you could check that instead of checking the
constructor
> > > name.
> > 
> > I have moved everything to a runtime function. Is there any overhead in
> calling
> > them? Should I first check class_variable_proxy() in case I can omit the
call?
> 
> I'll defer to Ignition experts to answer that --- I assume it's not as bad as
it
> is to call a runtime function from JIT code, but I don't know.

A runtime call is just as expensive as it would be from JIT code, so if we can
avoid generating one when we know statically from the parser that it's not
needed then that would be good.

[Henrique Ferreiro, 2016-11-11 16:24:52]

On 2016/10/24 at 15:56:10, caitp wrote:
>
https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
> src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
> On 2016/10/17 11:41:11, hferreiro wrote:
> > Should I use a runtime function to check for this?
> 
> It looks like expr->class_variable_proxy() returns nullptr for anonymous
classes, so I think you could check that instead of checking the constructor
name.

The problem is that it is also null for "var a = class {}", I will use
constructor()->raw_name()->IsEmpty(), then.

[rmcilroy, 2016-11-14 13:22:18]

interpreter/ changes LGTM with a couple of nits, but didn't look at the rest.

https://codereview.chromium.org/2423053002/diff/40001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2423053002/diff/40001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1903: void
FullCodeGenerator::EmitClassDefineNameProperty(ClassLiteral* lit) {
After the change to send all TurboFan code via Ignition (rather than
full-codegen) I don't think we will ever see class definitions in full-codegen. 

Benedkit: Do we need to add this support to FCG assuming the Ignition for
Turbofan code CL sticks (which it seems to be).

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

https://codereview.chromium.org/2423053002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:574:
prototype_string_(info->isolate()->factory()->prototype_string()) {}
nit - did "git cl" do this? If not, please revert.

https://codereview.chromium.org/2423053002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1498:
DCHECK(expr->constructor()->raw_name() != nullptr);
no point having this DCHECK since the next line will crash if it fails anyway.
Please remove

[Toon Verwaest, 2016-11-14 14:52:08]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-11-14 14:52:09]

Is there a design doc for this? If not, could you give some insights into how
this works and will work in the future?

Isn't the most common case that the name of the class is the same as the one
from the SharedFunctionInfo, and hence we could keep on using the regular
JSFunction::name accessor rather than installing a data property? Probably in
most cases we even know this at compile-time of the class literal, and don't
need the DefineClassNameProperty call at the end.

If the name could be, or is custom, we know that up-front as well. In that case
we can use a version of the class JSFunction that has place for an inobject name
property. Since we support subclassing of builtins we don't need to stick to
JSFunction::kSize anymore. We just need to make a new JSFunction-map for classes
with a name that declare the additional space.

[Henrique Ferreiro, 2016-11-14 23:19:32]

On 2016/11/14 14:52:09, Toon Verwaest wrote:
> Is there a design doc for this? If not, could you give some insights into how
> this works and will work in the future?
> 
> Isn't the most common case that the name of the class is the same as the one
> from the SharedFunctionInfo, and hence we could keep on using the regular
> JSFunction::name accessor rather than installing a data property? Probably in
> most cases we even know this at compile-time of the class literal, and don't
> need the DefineClassNameProperty call at the end.
> 
> If the name could be, or is custom, we know that up-front as well. In that
case
> we can use a version of the class JSFunction that has place for an inobject
name
> property. Since we support subclassing of builtins we don't need to stick to
> JSFunction::kSize anymore. We just need to make a new JSFunction-map for
classes
> with a name that declare the additional space.

Hi, I am a new contributor to V8 so I don't understand much of what you are
asking. This CL was sent to fix bug v8:4199, which states that the standard
requires a name() property to be added last, when defining the class, unless it
already exists such a static function. Can you clarify what alternative option
you are proposing or what is wrong with this approach?

[caitp, 2016-11-14 23:25:39]

On 2016/11/14 23:19:32, hferreiro wrote:
> On 2016/11/14 14:52:09, Toon Verwaest wrote:
> > Is there a design doc for this? If not, could you give some insights into
how
> > this works and will work in the future?
> > 
> > Isn't the most common case that the name of the class is the same as the one
> > from the SharedFunctionInfo, and hence we could keep on using the regular
> > JSFunction::name accessor rather than installing a data property? Probably
in
> > most cases we even know this at compile-time of the class literal, and don't
> > need the DefineClassNameProperty call at the end.
> > 
> > If the name could be, or is custom, we know that up-front as well. In that
> case
> > we can use a version of the class JSFunction that has place for an inobject
> name
> > property. Since we support subclassing of builtins we don't need to stick to
> > JSFunction::kSize anymore. We just need to make a new JSFunction-map for
> classes
> > with a name that declare the additional space.
> 
> Hi, I am a new contributor to V8 so I don't understand much of what you are
> asking. This CL was sent to fix bug v8:4199, which states that the standard
> requires a name() property to be added last, when defining the class, unless
it
> already exists such a static function. Can you clarify what alternative option
> you are proposing or what is wrong with this approach?

He's saying (correct me if I'm mistaken), _if_ there is no static method with a
computed property name, _and_ no static method with a literal property name ===
"name", you can get away with using the regular strict function Map.

I think doing this would still fail those test262 tests which assert the
enumerated order of properties, though, so if we want to do that for the
performance win, it will be a deliberate spec violation.

[Toon Verwaest, 2016-11-15 08:53:04]

You can't use that map, it needs to be a custom map with the name getter at the
end. But you don't need to install it as a data property like you're doing here.

The second part is that by adding a data property in the way you're doing it,
you're making it an out-of-object property. That unnecessarily increases the
size of every class by 5 words. (a fixed array with space for 3 properties +
header which is 2 words) If you do assume you need it, because there's a
computed property, you can use a different map that provides 1 word additional
space in the class to store the name.

If you know you don't need it, because there's a static property, you can use
another custom map that doesn't have it.

Of course the second point applies to all static properties; and if they are
currently all out-of-object properties you can significantly benefit by
implementing this properly. I guess point 2 and 3 are independent of this CL.
The important part for this CL is point 1: don't install a data property if you
can keep on using an accessor.

[Henrique Ferreiro, 2016-11-15 22:38:24]

On 2016/11/15 at 08:53:04, verwaest wrote:
> You can't use that map, it needs to be a custom map with the name getter at
the end. But you don't need to install it as a data property like you're doing
here.
> 
> The second part is that by adding a data property in the way you're doing it,
you're making it an out-of-object property. That unnecessarily increases the
size of every class by 5 words. (a fixed array with space for 3 properties +
header which is 2 words) If you do assume you need it, because there's a
computed property, you can use a different map that provides 1 word additional
space in the class to store the name.
> 
> If you know you don't need it, because there's a static property, you can use
another custom map that doesn't have it.
> 
> Of course the second point applies to all static properties; and if they are
currently all out-of-object properties you can significantly benefit by
implementing this properly. I guess point 2 and 3 are independent of this CL.
The important part for this CL is point 1: don't install a data property if you
can keep on using an accessor.

So, what you want me to do here is to install 'name' as an accessor, using a new
class index map which already has the space allocated?

[Toon Verwaest, 2016-11-16 12:12:47]

I see, the other static properties need to be in between. In that case just
install the FunctionName accessor if it's not masking anything and the name is
the literal name from the SharedFunctionInfo.

In the longer run we should have inobject space for all static properties, which
can include space for a custom name in case it differs from the SFI name.

[Henrique Ferreiro, 2016-11-17 22:26:39]

On 2016/11/16 at 12:12:47, verwaest wrote:
> I see, the other static properties need to be in between. In that case just
install the FunctionName accessor if it's not masking anything and the name is
the literal name from the SharedFunctionInfo.

I have just done that. I imagine you would prefer if the check for an already
existing static method named 'name' is done statically, while processing the
properties, right?

> In the longer run we should have inobject space for all static properties,
which can include space for a custom name in case it differs from the SFI name.

Not sure if that would be possible though, because the order of the properties
would be dependent on how they were declared in the class.

[Henrique Ferreiro, 2016-11-17 22:31:03]

On 2016/11/17 at 22:26:39, hferreiro wrote:
> On 2016/11/16 at 12:12:47, verwaest wrote:
> > I see, the other static properties need to be in between. In that case just
install the FunctionName accessor if it's not masking anything and the name is
the literal name from the SharedFunctionInfo.

Is it possible for the name to be different from the SharedFunctionInfo recorded
name?

[Toon Verwaest, 2016-11-24 07:13:38]

Thanks, this already looks much better! Another comment.
(Sorry for slow-poke review, I somehow missed this...)

https://codereview.chromium.org/2423053002/diff/60001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2423053002/diff/60001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1906: if
(!lit->constructor()->raw_name()->IsEmpty()) {
Here you can know at compile-time that the name is the empty string.

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

https://codereview.chromium.org/2423053002/diff/60001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:200: if (name->length() == 0) return *object;
You can check this at compile-time, which means you don't need to pass 'name'
into DefineClassNameProperty.

[Toon Verwaest, 2016-11-24 07:16:23]

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1625: if
(expr->constructor()->name()->length() == 0) return;
On 2016/11/10 12:37:44, rmcilroy wrote:
> On 2016/11/08 22:59:40, caitp wrote:
> > On 2016/11/08 22:28:58, hferreiro wrote:
> > > On 2016/10/24 15:56:10, caitp wrote:
> > > > On 2016/10/17 11:41:11, hferreiro wrote:
> > > > > Should I use a runtime function to check for this?
> > > > 
> > > > It looks like expr->class_variable_proxy() returns nullptr for anonymous
> > > > classes, so I think you could check that instead of checking the
> constructor
> > > > name.
> > > 
> > > I have moved everything to a runtime function. Is there any overhead in
> > calling
> > > them? Should I first check class_variable_proxy() in case I can omit the
> call?
> > 
> > I'll defer to Ignition experts to answer that --- I assume it's not as bad
as
> it
> > is to call a runtime function from JIT code, but I don't know.
> 
> A runtime call is just as expensive as it would be from JIT code, so if we can
> avoid generating one when we know statically from the parser that it's not
> needed then that would be good.

This is exactly what I was referring to. Checking it without dereferencing the
handle is easy because the AstValueFactory has the empty string; plus you can
just use IsEmpty() on the string as well.

[rmcilroy, 2016-11-24 14:31:15]

https://codereview.chromium.org/2423053002/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2423053002/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1581: EmitClassDefineNameProperty(lit);
Just a heads up - FullCodeGenerator::VisitClassLiteral is now dead code since
any code which is unsupported by Crankshaft is now only ever executed in
Ignition+TurboFan (not FCG), so no need to add / port this code. We will get
round to ripping out the existing class literal code soon (see
https://bugs.chromium.org/p/v8/issues/detail?id=5657).

[Henrique Ferreiro, 2016-11-26 22:42:25]

I have added code to check for properties named 'name' at parse-time, and also
when evaluating properties at runtime, so that the runtime call to install the
name accessor can be avoided in more cases. @rmcilroy: this would need a new
review of the interpreter changes.

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

https://codereview.chromium.org/2423053002/diff/80001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:202: CHECK(!JSObject::SetAccessor(
Is this comment and CHECK still valid using SetAccessor?

[rmcilroy, 2016-11-28 10:28:59]

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

https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1394: Register needs_name = args[2];
This shouldn't be allocated in the same RegisterList as args since it's never
passed to a runtime call (actually looking at this, I'm not sure why we use a
RegisterList for this at all any more). It should just be a single register.
Saying that, I'm not keen on this approach of passing a register as a boolean
value (see below).

https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1468: }
I'm not keen on this extra code added to every property initialization. Do we
need it, or could we just rely on the compile time checks (e.g.,
expr->constructor()->raw_name()->IsEmpty() etc.) and have the runtime call check
if there was a computed property name which added the name property ?

[Toon Verwaest, 2016-11-28 10:40:58]

https://codereview.chromium.org/2423053002/diff/80001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2423053002/diff/80001/src/ast/ast.h#newcode2862
src/ast/ast.h:2862: bool has_name_static_property_;
This should be added to the bitfield provided by AstNode. Check other subclasses
to see how it's done.

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

https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1436: bool check_name =
!expr->has_name_static_property() &&
Isn't the general case that we don't have a static computed property name? If
so, we should avoid this additional work in that case.

https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1468: }
On 2016/11/28 10:28:59, rmcilroy wrote:
> I'm not keen on this extra code added to every property initialization. Do we
> need it, or could we just rely on the compile time checks (e.g.,
> expr->constructor()->raw_name()->IsEmpty() etc.) and have the runtime call
check
> if there was a computed property name which added the name property ?

I agree, we should just do something more expensive in the runtime if this was
the case. It shouldn't be the common case anyway.

We can add it if we decide we it's common enough to avoid the runtime call
altogether.

https://codereview.chromium.org/2423053002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2423053002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:2148: DCHECK(has_name_static_property != nullptr);
DCHECK_NOT_NULL

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

https://codereview.chromium.org/2423053002/diff/80001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:202: CHECK(!JSObject::SetAccessor(
On 2016/11/26 22:42:24, hferreiro wrote:
> Is this comment and CHECK still valid using SetAccessor?

Yes

[Henrique Ferreiro, 2016-11-28 11:25:04]

>
https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:1468: }
> I'm not keen on this extra code added to every property initialization. Do we
> need it, or could we just rely on the compile time checks (e.g.,
> expr->constructor()->raw_name()->IsEmpty() etc.) and have the runtime call
check
> if there was a computed property name which added the name property ?

This code is only added to computed name properties, which should not be that
common, are they? Otherwise, I can do the check in the runtime function, before
installing the property. In that case, would it be worth it to check in the
parser if there are computed property names, and only do the runtime check in
that case?

[rmcilroy, 2016-11-28 13:58:27]

On 2016/11/28 11:25:04, hferreiro wrote:
> >
>
https://codereview.chromium.org/2423053002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-generator.cc:1468: }
> > I'm not keen on this extra code added to every property initialization. Do
we
> > need it, or could we just rely on the compile time checks (e.g.,
> > expr->constructor()->raw_name()->IsEmpty() etc.) and have the runtime call
> check
> > if there was a computed property name which added the name property ?
> 
> This code is only added to computed name properties, which should not be that
> common, are they? Otherwise, I can do the check in the runtime function,
before
> installing the property. In that case, would it be worth it to check in the
> parser if there are computed property names, and only do the runtime check in
> that case?

Personally I would just always make the runtime call if there are computed
property names (given they are not too common), and add the extra checks in the
runtime call to check if name was added as a computed property.

[Henrique Ferreiro, 2016-11-29 11:01:26]

I moved the check to the runtime function, but added a boolean to omit it in
case we didn't find any static computed name properties in the parser.

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:84: BytecodeArrayBuilder&
LoadBoolean(bool is_true) {
I have added this helper.

https://codereview.chromium.org/2423053002/diff/100001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:2151: DCHECK_NOT_NULL(has_seen_constructor);
Is it ok to fix this in this CL?

[Toon Verwaest, 2016-11-29 12:27:47]

Looking good. Some final comments.

https://codereview.chromium.org/2423053002/diff/100001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/ast/ast.h#newcode2855
src/ast/ast.h:2855: HasNameStaticProperty::update(bit_field_,
has_name_static_property) |
HasNameStaticProperty::encode(has_name_static_property) | ...

https://codereview.chromium.org/2423053002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2423053002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1685:
jsgraph()->BooleanConstant(expr->has_static_computed_names()));
The standard way is to call a different runtime function rather than passing in
a boolean. You can make the C++ side share code obviously.

https://codereview.chromium.org/2423053002/diff/100001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:2151: DCHECK_NOT_NULL(has_seen_constructor);
On 2016/11/29 11:01:26, hferreiro wrote:
> Is it ok to fix this in this CL?

Sure, drive-by fixes like this are entirely fine imho.

[rmcilroy, 2016-11-29 13:05:23]

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:84: BytecodeArrayBuilder&
LoadBoolean(bool is_true) {
On 2016/11/29 11:01:26, hferreiro wrote:
> I have added this helper.

Acknowledged.

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1505: .StoreAccumulatorInRegister(args[1])
Rather than storing a compile time known boolean as an argument to the runtime
call, how about just creating two different runtime calls - one which does the
check and one which doesn't? Then the code here could be something like:

RuntimeId runtime_id = expr->has_static_computed_names() 
  ? Runtime::kInstallClassNameAccessorWithComputedNameCheck
  : Runtime::kInstallClassNameAccessor
builder()->CallRuntime(runtime_id, literal);

Which would be much fewer bytes. 

Toon - any objection to having two runtime calls for with and without the check
?

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2423053002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:136: void
VisitClassLiteralStaticPrototypeWithComputedName(Register name);
Unused? Please remove.

[Henrique Ferreiro, 2016-12-01 16:06:51]

I have added a new runtime function InstallNameAccessorWithCheck.

[Toon Verwaest, 2016-12-01 22:14:22]

rmcilroy: nothing against it at all; that's what I meant to suggest in my
previous comment.

This version lgtm. Thanks Henrique!

[caitp, 2016-12-01 22:15:49]

On 2016/12/01 22:14:22, Toon Verwaest wrote:
> rmcilroy: nothing against it at all; that's what I meant to suggest in my
> previous comment.
> 
> This version lgtm. Thanks Henrique!

Before landing this, I think the full-codegen changes (and mention in the CL
description) should be removed

[rmcilroy, 2016-12-01 23:59:46]

Interpreter changes LGTM, thanks for the changes.

> Before landing this, I think the full-codegen changes (and mention in the CL
> description) should be removed

Yes please remove the Full-codegen and AstGraphBuilder changes since neither of
those deal with classes any longer.

[Henrique Ferreiro, 2016-12-02 13:02:32]

I have rebased my changes.

https://codereview.chromium.org/2423053002/diff/140001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2423053002/diff/140001/src/parsing/parser-bas...
src/parsing/parser-base.h:4158: if (!class_info.has_static_computed_names &&
is_static &&
I moved this out of ParseClassPropertyDefinition given that it now also returns
is_static.

[Henrique Ferreiro, 2016-12-02 13:02:55]

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

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

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-02 13:21:50]

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

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

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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Henrique Ferreiro, 2016-12-02 22:48:17]

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

[commit-bot: I haz the power, 2016-12-02 22:48: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-02 23:20:10]

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

[commit-bot: I haz the power, 2016-12-02 23:20:11]

Dry run: This issue passed the CQ dry run.

[Henrique Ferreiro, 2016-12-03 21:29:11]

Description was changed from

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

Implementations in fullcodegen are missing for most architectures pending
review.

BUG=v8:4199
==========

to

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

BUG=v8:4199
==========

[Toon Verwaest, 2016-12-05 15:18:19]

lgtm

[Henrique Ferreiro, 2016-12-06 20:40:00]

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

[Henrique Ferreiro, 2016-12-06 20:40:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2423053002/#ps160001
(title: "Fix runtime function argument count")

[commit-bot: I haz the power, 2016-12-06 20:40:05]

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-06 21:10:05]

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

[commit-bot: I haz the power, 2016-12-06 21:10:07]

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

Patch:       src/contexts.h
Index: src/contexts.h
diff --git a/src/contexts.h b/src/contexts.h
index
77ff09c464ea7f14872b811536dba50fdae23999..2a1657f51e2de02871d1924c9d92cc167bf73686
100644
--- a/src/contexts.h
+++ b/src/contexts.h
@@ -324,6 +324,7 @@ enum ContextLookupFlags {
   V(STRICT_FUNCTION_WITHOUT_PROTOTYPE_MAP_INDEX, Map,                         
\
     strict_function_without_prototype_map)                                    
\
   V(STRICT_GENERATOR_FUNCTION_MAP_INDEX, Map, strict_generator_function_map)  
\
+  V(CLASS_FUNCTION_MAP_INDEX, Map, class_function_map)                        
\
   V(STRING_FUNCTION_INDEX, JSFunction, string_function)                       
\
   V(STRING_FUNCTION_PROTOTYPE_MAP_INDEX, Map, string_function_prototype_map)  
\
   V(SYMBOL_FUNCTION_INDEX, JSFunction, symbol_function)                       
\
@@ -616,8 +617,10 @@ class Context: public FixedArray {
     }
 
     if (IsClassConstructor(kind)) {
-      // Use strict function map (no own "caller" / "arguments")
-      return STRICT_FUNCTION_MAP_INDEX;
+      // Like the strict function map, but with no 'name' accessor. 'name'
+      // needs to be the last property and it is added during instantiation,
+      // in case a static property with the same name exists"
+      return CLASS_FUNCTION_MAP_INDEX;
     }
 
     if (IsArrowFunction(kind) || IsConciseMethod(kind) ||

[Henrique Ferreiro, 2016-12-07 10:05:35]

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

[Henrique Ferreiro, 2016-12-07 10:05:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2423053002/#ps180001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-07 10:05:38]

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-07 10:34:16]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481105135655980,
"parent_rev": "ff9f7feda6b854d3eedb8bbb4d61b4e76ccd8bb2", "commit_rev":
"bcf69c2f74573f28a566f9bd584289fd80aa78eb"}

[commit-bot: I haz the power, 2016-12-07 10:34:20]

Description was changed from

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

BUG=v8:4199
==========

to

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

BUG=v8:4199
==========

[commit-bot: I haz the power, 2016-12-07 10:34:22]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-07 10:35:08]

Description was changed from

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

BUG=v8:4199
==========

to

==========
Install the 'name' property in classes at runtime

This allows to detect a static property also named 'name', and also makes sure
'name' is added last, to be standards-compliant.

BUG=v8:4199

Committed: https://crrev.com/afd5ff553bbc8abdee37661d6a88af253673aa7c
Cr-Commit-Position: refs/heads/master@{#41546}
==========

[commit-bot: I haz the power, 2016-12-07 10:35:10]

Patchset 10 (id:??) landed as
https://crrev.com/afd5ff553bbc8abdee37661d6a88af253673aa7c
Cr-Commit-Position: refs/heads/master@{#41546}