Classes runtime

This uses a runtime function to set up the the constructor and its
prototype.

This does not add the methods/accessors to the prototype or the
constructor.

BUG=v8:3330
LOG=Y
R=dslomov@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24442

[arv (Not doing code reviews), 2014-10-03 21:01:01]

arv@chromium.org changed reviewers:
+ adamk@chromium.org, dslomov@chromium.org

[arv (Not doing code reviews), 2014-10-03 21:11:57]

Ready for review.

https://codereview.chromium.org/631433002/diff/1/src/parser.cc
File src/parser.cc (left):

https://codereview.chromium.org/631433002/diff/1/src/parser.cc#oldcode1977
src/parser.cc:1977: Block* block = factory()->NewBlock(NULL, 1, true, pos);
No need for this block since we only need to return one statement here.

[arv (Not doing code reviews), 2014-10-03 21:20:00]

https://codereview.chromium.org/631433002/diff/1/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/631433002/diff/1/src/compiler/pipeline.cc#new...
src/compiler/pipeline.cc:176: info()->function()->dont_optimize_reason() ==
kClassLiteral ||
Why isn't this sufficient?

https://codereview.chromium.org/631433002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/631433002/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:74: 'harmony/classes': [PASS, NO_VARIANTS],
I'm not sure why the change in pipeline.cc is not sufficient?

[Dmitry Lomov (no reviews), 2014-10-06 11:04:12]

On 2014/10/03 21:20:00, arv wrote:
> https://codereview.chromium.org/631433002/diff/1/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/631433002/diff/1/src/compiler/pipeline.cc#new...
> src/compiler/pipeline.cc:176: info()->function()->dont_optimize_reason() ==
> kClassLiteral ||
> Why isn't this sufficient?
> 
> https://codereview.chromium.org/631433002/diff/1/test/mjsunit/mjsunit.status
> File test/mjsunit/mjsunit.status (right):
> 
>
https://codereview.chromium.org/631433002/diff/1/test/mjsunit/mjsunit.status#...
> test/mjsunit/mjsunit.status:74: 'harmony/classes': [PASS, NO_VARIANTS],
> I'm not sure why the change in pipeline.cc is not sufficient?

I applied the patch locally and what I see is:
#
# Fatal error in ../src/full-codegen.cc, line 460
# CHECK(bailout_entries_[i].id != id) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2:
v8::internal::FullCodeGenerator::PrepareForBailoutForId(v8::internal::BailoutId,
v8::internal::FullCodeGenerator::State)
 3:
v8::internal::FullCodeGenerator::PrepareForBailout(v8::internal::Expression*,
v8::internal::FullCodeGenerator::State)
 4: v8::internal::FullCodeGenerator::VisitAssignment(v8::internal::Assignment*)
 5:
v8::internal::FullCodeGenerator::VisitExpressionStatement(v8::internal::ExpressionStatement*)
 6:
v8::internal::AstVisitor::VisitStatements(v8::internal::ZoneList<v8::internal::Statement*>*)
 7: v8::internal::FullCodeGenerator::Generate()
 8: v8::internal::FullCodeGenerator::MakeCode(v8::internal::CompilationInfo*)
 9:
v8::internal::Compiler::EnsureDeoptimizationSupport(v8::internal::CompilationInfo*)
10: v8::internal::OptimizedCompileJob::CreateGraph()
11: ??
12:
v8::internal::Compiler::GetOptimizedCode(v8::internal::Handle<v8::internal::JSFunction>,
v8::internal::Handle<v8::internal::Code>,
v8::internal::Compiler::ConcurrencyMode, v8::internal::BailoutId)
13: v8::internal::Runtime_CompileOptimized(int, v8::internal::Object**,
v8::internal::Isolate*)
14: ??
15: ??
16: ??
17: ??
18: ??
19: ??
Command: /usr/local/google/home/dslomov/work/v8/out/ia32.optdebug/d8 --test
--random-seed=-2046048691 --stress-opt --always-opt --nohard-abort
--nodead-code-elimination --nofold-constants --enable-slow-asserts --debug-code
--verify-heap --harmony
/usr/local/google/home/dslomov/work/v8/test/mjsunit/mjsunit.js
/usr/local/google/home/dslomov/work/v8/test/mjsunit/harmony/classes.js

This has nothing to do with TurboFan - this means that you use bailout ids
twice. Not sure yet how this happened - I'll have investigate.

[Dmitry Lomov (no reviews), 2014-10-06 14:20:59]

Ok I figured out the BailoutId clash. The minimal test case is minimal:
function foobar() {
  var C = class  { }
}

foobar();

Just run it with "--harmony-classes --always-opt".
The (incomplete) fix is:
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -2754,11 +2754,13 @@ typename ParserBase<Traits>::ExpressionT
ParserBase<Traits>::ParseClassLiteral(
 
   // TODO(arv): Implement scopes and name binding in class body only.
   // TODO(arv): Maybe add CLASS_SCOPE?
-  typename Traits::Type::ScopePtr extends_scope =
-      this->NewScope(scope_, BLOCK_SCOPE);
+  // typename Traits::Type::ScopePtr extends_scope =
+  //    this->NewScope(scope_, BLOCK_SCOPE);
+  /*
   FunctionState extends_function_state(
       &function_state_, &scope_, &extends_scope, zone(),
       this->ast_value_factory(), ast_node_id_gen_);
+  */
   scope_->SetStrictMode(STRICT);
   scope_->SetScopeName(name);
 
The FunctionState you are using here to enter the scope has a side effect of
resetting the ast_node_id_gen (the bailout id generator).
What should happen here instead is just entering the scopes without using
FunctionState.

[arv (Not doing code reviews), 2014-10-06 14:50:41]

On 2014/10/06 14:20:59, Dmitry Lomov (chromium) wrote:
> Ok I figured out the BailoutId clash. The minimal test case is minimal:
> function foobar() {
>   var C = class  { }
> }
> 
> foobar();
> 
> Just run it with "--harmony-classes --always-opt".
> The (incomplete) fix is:
> --- a/src/preparser.h
> +++ b/src/preparser.h
> @@ -2754,11 +2754,13 @@ typename ParserBase<Traits>::ExpressionT
> ParserBase<Traits>::ParseClassLiteral(
>  
>    // TODO(arv): Implement scopes and name binding in class body only.
>    // TODO(arv): Maybe add CLASS_SCOPE?
> -  typename Traits::Type::ScopePtr extends_scope =
> -      this->NewScope(scope_, BLOCK_SCOPE);
> +  // typename Traits::Type::ScopePtr extends_scope =
> +  //    this->NewScope(scope_, BLOCK_SCOPE);
> +  /*
>    FunctionState extends_function_state(
>        &function_state_, &scope_, &extends_scope, zone(),
>        this->ast_value_factory(), ast_node_id_gen_);
> +  */
>    scope_->SetStrictMode(STRICT);
>    scope_->SetScopeName(name);
>  
> The FunctionState you are using here to enter the scope has a side effect of
> resetting the ast_node_id_gen (the bailout id generator).
> What should happen here instead is just entering the scopes without using
> FunctionState.

Thanks for looking into this.

I need to enter some scope to change the code to be strict. I'll see what needs
to be done to achieve that.

[arv (Not doing code reviews), 2014-10-06 19:20:53]

Fix issue with function state

[arv (Not doing code reviews), 2014-10-06 19:24:08]

I fixed the issue with the FunctionState and bailout id.

This does not fix the issue that compiler::AstGraphBuilder::VisitClassLiteral is
reached if I revert the change to mjsunit.status.

[Dmitry Lomov (no reviews), 2014-10-07 09:26:08]

On 2014/10/06 19:24:08, arv wrote:
> I fixed the issue with the FunctionState and bailout id.
> 
> This does not fix the issue that compiler::AstGraphBuilder::VisitClassLiteral
is
> reached if I revert the change to mjsunit.status.

Yes, the assert is there because of this test:
(function TestStrictMode() {
  class C {}

  with ({a: 1}) {
    assertEquals(1, a);
  }

  assertThrows('class C extends function B() { with ({}); return B; }() {}',
               SyntaxError);

})();

TestStrictMode has _two_ dont_optimize_reasons - one is 'with' and another is
class literal. 'with' "wins" due to the way the visitor visits, so the
pipeline.cc check tries to optimize. Let's split this test into a separate file
and disable it until turbo fan has a fix.

[Dmitry Lomov (no reviews), 2014-10-07 09:26:09]

On 2014/10/06 19:24:08, arv wrote:
> I fixed the issue with the FunctionState and bailout id.
> 
> This does not fix the issue that compiler::AstGraphBuilder::VisitClassLiteral
is
> reached if I revert the change to mjsunit.status.

Yes, the assert is there because of this test:
(function TestStrictMode() {
  class C {}

  with ({a: 1}) {
    assertEquals(1, a);
  }

  assertThrows('class C extends function B() { with ({}); return B; }() {}',
               SyntaxError);

})();

TestStrictMode has _two_ dont_optimize_reasons - one is 'with' and another is
class literal. 'with' "wins" due to the way the visitor visits, so the
pipeline.cc check tries to optimize. Let's split this test into a separate file
and disable it until turbo fan has a fix.

[Dmitry Lomov (no reviews), 2014-10-07 10:28:33]

Looking good, some comments

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode153
src/preparser.h:153: *scope_stack_ = scope_;
Why do you need this constructor?

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode2765
src/preparser.h:2765: BlockState block_state(&scope_, &scope);
Seems like BlockState block_state(&scope_, scope) will do just fine here.

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:376: prototype_parent =
isolate->initial_object_prototype();
What happens, per spec, if user assigns to Object.prototype?

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:405: isolate, JSObject::SetPrototype(prototype,
prototype_parent, false));
Instead of SetPrototype here, I suggest using NewJSObjectFromMap with an
appropriate Map (the prototype field of which is set to prototype_parent), so
that we can avoid prototype mutation.

[Dmitry Lomov (no reviews), 2014-10-07 11:26:47]

On 2014/10/07 09:26:09, Dmitry Lomov (chromium) wrote:
> On 2014/10/06 19:24:08, arv wrote:
> > I fixed the issue with the FunctionState and bailout id.
> > 
> > This does not fix the issue that
compiler::AstGraphBuilder::VisitClassLiteral
> is
> > reached if I revert the change to mjsunit.status.
> 
> Yes, the assert is there because of this test:
> (function TestStrictMode() {
>   class C {}
> 
>   with ({a: 1}) {
>     assertEquals(1, a);
>   }
> 
>   assertThrows('class C extends function B() { with ({}); return B; }() {}',
>                SyntaxError);
> 
> })();
> 
> TestStrictMode has _two_ dont_optimize_reasons - one is 'with' and another is
> class literal. 'with' "wins" due to the way the visitor visits, so the
> pipeline.cc check tries to optimize. Let's split this test into a separate
file
> and disable it until turbo fan has a fix.

Nevermind, already fixing that in a separate CL.

[arv (Not doing code reviews), 2014-10-07 14:46:04]

Create prototype object with initial prototype parent

[arv (Not doing code reviews), 2014-10-07 14:47:10]

PTAL

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode153
src/preparser.h:153: *scope_stack_ = scope_;
On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> Why do you need this constructor?

This is due to how parser and preparser typedef Scope differently.

We have the same duplication of the constructor for FunctionState.

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode2765
src/preparser.h:2765: BlockState block_state(&scope_, &scope);
On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> Seems like BlockState block_state(&scope_, scope) will do just fine here.

That works with PreParser but not with Parser.

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:376: prototype_parent =
isolate->initial_object_prototype();
On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> What happens, per spec, if user assigns to Object.prototype?

The spec uses %ObjectPrototype% which is the original Object.prototype.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-c...

https://codereview.chromium.org/631433002/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:405: isolate, JSObject::SetPrototype(prototype,
prototype_parent, false));
On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> Instead of SetPrototype here, I suggest using NewJSObjectFromMap with an
> appropriate Map (the prototype field of which is set to prototype_parent), so
> that we can avoid prototype mutation.

Done.

I used to have a todo for this... I'm not sure when the todo dissappeared...
well now it is done.

[Dmitry Lomov (no reviews), 2014-10-07 15:09:37]

lgtm with nits.

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode153
src/preparser.h:153: *scope_stack_ = scope_;
On 2014/10/07 14:47:09, arv wrote:
> On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> > Why do you need this constructor?
> 
> This is due to how parser and preparser typedef Scope differently.
> 
> We have the same duplication of the constructor for FunctionState.

Could we investigate the possibility of removing this discrepancy between
prepaser and parser? This is very confusing (not necessarily in this CL)

https://codereview.chromium.org/631433002/diff/40001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/631433002/diff/40001/src/globals.h#newcode769
src/globals.h:769: kConstructorMethod = kNormalFunction
Set but unused? Let's remove it.

https://codereview.chromium.org/631433002/diff/40001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/631433002/diff/40001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:433: if (has_constructor_parent) {
Replace with constructor_parent.is_null() and remove flag?

https://codereview.chromium.org/631433002/diff/40001/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/631433002/diff/40001/test/mjsunit/mjsunit.sta...
test/mjsunit/mjsunit.status:74: 'harmony/classes': [PASS, NO_VARIANTS],
You should not need this after
https://code.google.com/p/v8/source/detail?r=24435.

Don't forget to remove after you rebase.

[arv (Not doing code reviews), 2014-10-07 15:16:39]

Remove has_constructor_parent

[arv (Not doing code reviews), 2014-10-07 15:16:52]

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/631433002/diff/20001/src/preparser.h#newcode153
src/preparser.h:153: *scope_stack_ = scope_;
On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> On 2014/10/07 14:47:09, arv wrote:
> > On 2014/10/07 10:28:32, Dmitry Lomov (chromium) wrote:
> > > Why do you need this constructor?
> > 
> > This is due to how parser and preparser typedef Scope differently.
> > 
> > We have the same duplication of the constructor for FunctionState.
> 
> Could we investigate the possibility of removing this discrepancy between
> prepaser and parser? This is very confusing (not necessarily in this CL)

I agree that this is confusing. I struggled quite a bit to understand what was
going on here.

https://codereview.chromium.org/631433002/diff/40001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/631433002/diff/40001/src/globals.h#newcode769
src/globals.h:769: kConstructorMethod = kNormalFunction
On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> Set but unused? Let's remove it.

It is used in preparser.h. I can change that to use kNormalFunction but I found
that this makes it more clear what is going on.

https://codereview.chromium.org/631433002/diff/40001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/631433002/diff/40001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:433: if (has_constructor_parent) {
On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> Replace with constructor_parent.is_null() and remove flag?

Good idea. Done.

https://codereview.chromium.org/631433002/diff/40001/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/631433002/diff/40001/test/mjsunit/mjsunit.sta...
test/mjsunit/mjsunit.status:74: 'harmony/classes': [PASS, NO_VARIANTS],
On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> You should not need this after
> https://code.google.com/p/v8/source/detail?r=24435.
> 
> Don't forget to remove after you rebase.

Acknowledged.

[Dmitry Lomov (no reviews), 2014-10-07 15:21:05]

https://codereview.chromium.org/631433002/diff/40001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/631433002/diff/40001/src/globals.h#newcode769
src/globals.h:769: kConstructorMethod = kNormalFunction
On 2014/10/07 15:16:52, arv wrote:
> On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> > Set but unused? Let's remove it.
> 
> It is used in preparser.h. I can change that to use kNormalFunction but I
found
> that this makes it more clear what is going on.

Right, I've seen that. Let's not add constants that have no operational meaning
- it is just confusing - one has to look up whether constructor method is
somehow different from normal method.

[arv (Not doing code reviews), 2014-10-07 15:38:49]

REbase and revert mjsunit.status

[arv (Not doing code reviews), 2014-10-07 15:43:03]

Remove kConstructorFunction

[arv (Not doing code reviews), 2014-10-07 16:13:19]

https://codereview.chromium.org/631433002/diff/40001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/631433002/diff/40001/src/globals.h#newcode769
src/globals.h:769: kConstructorMethod = kNormalFunction
On 2014/10/07 15:21:05, Dmitry Lomov (chromium) wrote:
> On 2014/10/07 15:16:52, arv wrote:
> > On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> > > Set but unused? Let's remove it.
> > 
> > It is used in preparser.h. I can change that to use kNormalFunction but I
> found
> > that this makes it more clear what is going on.
> 
> Right, I've seen that. Let's not add constants that have no operational
meaning
> - it is just confusing - one has to look up whether constructor method is
> somehow different from normal method.

Done.

[Dmitry Lomov (no reviews), 2014-10-07 16:15:39]

On 2014/10/07 16:13:19, arv wrote:
> https://codereview.chromium.org/631433002/diff/40001/src/globals.h
> File src/globals.h (right):
> 
> https://codereview.chromium.org/631433002/diff/40001/src/globals.h#newcode769
> src/globals.h:769: kConstructorMethod = kNormalFunction
> On 2014/10/07 15:21:05, Dmitry Lomov (chromium) wrote:
> > On 2014/10/07 15:16:52, arv wrote:
> > > On 2014/10/07 15:09:37, Dmitry Lomov (chromium) wrote:
> > > > Set but unused? Let's remove it.
> > > 
> > > It is used in preparser.h. I can change that to use kNormalFunction but I
> > found
> > > that this makes it more clear what is going on.
> > 
> > Right, I've seen that. Let's not add constants that have no operational
> meaning
> > - it is just confusing - one has to look up whether constructor method is
> > somehow different from normal method.
> 
> Done.

lgtm

[arv (Not doing code reviews), 2014-10-07 16:25:07]

Committed patchset #6 (id:100001) manually as 24442 (presubmit successful).