[modules] Basic support of exports

[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569
Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Committed: https://crrev.com/21cb1105471e0b94039ba4037c17f2cb57cb2a8f
Cr-Original-Commit-Position: refs/heads/master@{#39341}
Cr-Commit-Position: refs/heads/master@{#39352}

[neis, 2016-09-01 10:20:06]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 10:20:09]

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-09-01 10:22:19]

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

[commit-bot: I haz the power, 2016-09-01 10:22:20]

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

[neis, 2016-09-01 10:34:47]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 10:34:59]

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-09-01 10:37:47]

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

[commit-bot: I haz the power, 2016-09-01 10:37:48]

Dry run: Try jobs failed on following builders:
  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_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[neis, 2016-09-01 10:40:08]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 10:40: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-09-01 10:44:18]

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

[commit-bot: I haz the power, 2016-09-01 10:44:19]

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

[neis, 2016-09-01 11:52:20]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 11:52:23]

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-09-01 11:55:35]

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

[commit-bot: I haz the power, 2016-09-01 11:55:35]

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

[neis, 2016-09-01 13:27:52]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 13:27:58]

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-09-01 13:31:47]

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

[commit-bot: I haz the power, 2016-09-01 13:31:48]

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

[neis, 2016-09-01 14:54:12]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 14: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-09-01 15:00:02]

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

[commit-bot: I haz the power, 2016-09-01 15:00:03]

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

[adamk, 2016-09-01 23:13:32]

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

[adamk, 2016-09-01 23:13:34]

This looks pretty straightforward, actually, with the main warts being things we
already know are weird or temporary (like how the JSModule is passed around).

This'll obviously want an interpreter/OWNER to review as well as myself when
it's ready to go.

https://codereview.chromium.org/2302783002/diff/100001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2302783002/diff/100001/src/contexts.cc#newcod...
src/contexts.cc:127: DCHECK_NOT_NULL(current);  // XXX
In our offline discussion I said this was a bit bogus because of how objects
work, but I do see that the bootstrapper actually _does_ set this to NULL. That
seems really wrong to me, though, so I'd prefer to leave out this DCHECK, as
it'll start failing if we ever fix the bootstrapper.

https://codereview.chromium.org/2302783002/diff/100001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2302783002/diff/100001/src/factory.cc#newcode...
src/factory.cc:1704: Handle<Map> map = NewMap(JS_MODULE_TYPE, JSModule::kSize);
As discussed offline, the module map should in the strong roots list, not
created anew every time NewJSModule is called.

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

https://codereview.chromium.org/2302783002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:961: variable->mode() == CONST);
I think you already touched this DCHECK in a different patch, but as far as I
know there's no way to have a CONST function declaration.

https://codereview.chromium.org/2302783002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:985:
VisitForAccumulatorValue(decl->fun());
Add a DCHECK above here that the mode is LET?

[neis, 2016-09-02 10:09:38]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 10:09: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-09-02 10:21:12]

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

[commit-bot: I haz the power, 2016-09-02 10:21:14]

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...)

[neis, 2016-09-02 11:32:58]

https://codereview.chromium.org/2302783002/diff/100001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2302783002/diff/100001/src/contexts.cc#newcod...
src/contexts.cc:127: DCHECK_NOT_NULL(current);  // XXX
On 2016/09/01 23:13:34, adamk wrote:
> In our offline discussion I said this was a bit bogus because of how objects
> work, but I do see that the bootstrapper actually _does_ set this to NULL.
That
> seems really wrong to me, though, so I'd prefer to leave out this DCHECK, as
> it'll start failing if we ever fix the bootstrapper.

Done.

https://codereview.chromium.org/2302783002/diff/100001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2302783002/diff/100001/src/factory.cc#newcode...
src/factory.cc:1704: Handle<Map> map = NewMap(JS_MODULE_TYPE, JSModule::kSize);
On 2016/09/01 23:13:34, adamk wrote:
> As discussed offline, the module map should in the strong roots list, not
> created anew every time NewJSModule is called.

Done. But I can't claim I really understand what I'm doing there.

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

https://codereview.chromium.org/2302783002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:961: variable->mode() == CONST);
On 2016/09/01 23:13:34, adamk wrote:
> I think you already touched this DCHECK in a different patch, but as far as I
> know there's no way to have a CONST function declaration.

Done in the other CL.

https://codereview.chromium.org/2302783002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:985:
VisitForAccumulatorValue(decl->fun());
On 2016/09/01 23:13:34, adamk wrote:
> Add a DCHECK above here that the mode is LET?

Done.

[neis, 2016-09-02 11:34:21]

Description was changed from

==========
WIP

COMMIT=false
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and to the
interpreter.
==========

[neis, 2016-09-02 11:42:50]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and to the
interpreter.
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.
==========

[neis, 2016-09-02 11:42:50]

neis@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[neis, 2016-09-02 11:43:34]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569
==========

[neis, 2016-09-02 12:06:07]

https://codereview.chromium.org/2302783002/diff/140001/src/ast/modules.h
File src/ast/modules.h (right):

https://codereview.chromium.org/2302783002/diff/140001/src/ast/modules.h#newc...
src/ast/modules.h:98: 
If we have Serialize and Deserialize here for entries, why not also have
Serialize and Deserialize here for whole descriptors?

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (left):

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:746: // Build function context only if
there are context allocated variables.
This comment was not quite right: a scope may not have any context-allocated
variables but still require a context.

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

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:748: VisitNewLocalFunctionContext();
I'm not happy with this name, as it can also create a ScriptContext and (now) a
ModuleContext, not just a FunctionContext. Any suggestions?

https://codereview.chromium.org/2302783002/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2302783002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:706: 
Here we awkwardly declare the module parameter, which required several changes
in other files to allow parameters in module scope.

Alternatively, we could keep 0 parameters in the module scope and instead treat
module scopes as a special case in CompilationInfo::num_parameters:

int CompilationInfo::num_parameters() const {
  if (scope()->is_module_scope()) return 1;
  return !IsStub() ? scope()->num_parameters() : parameter_count_;
}

We don't need a declared parameter variable because we never use the variable
itself. We directly access the stack slot via Parameter(1) at the beginning of
the module code, and then immediately store the module object into the context
extension slot for future access.

This alternative is still somewhat ugly but perhaps less ugly.

[neis, 2016-09-02 12:06:15]

PTAL

[neis, 2016-09-02 12:07:45]

neis@chromium.org changed reviewers:
+ ulan@chromium.org

[neis, 2016-09-02 12:07:46]

+ulan for heap

[neis, 2016-09-02 12:08:10]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 12:08:19]

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-09-02 12:34:54]

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

[commit-bot: I haz the power, 2016-09-02 12:34:55]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-02 17:51:51]

lgtm

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

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:748: VisitNewLocalFunctionContext();
On 2016/09/02 12:06:07, neis wrote:
> I'm not happy with this name, as it can also create a ScriptContext and (now)
a
> ModuleContext, not just a FunctionContext. Any suggestions?

BuildNewContext?

[rmcilroy, 2016-09-05 11:36:07]

Nice, Interpreter changes LGTM, didn't look at the rest.

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

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:748: VisitNewLocalFunctionContext();
On 2016/09/02 17:51:51, adamk wrote:
> On 2016/09/02 12:06:07, neis wrote:
> > I'm not happy with this name, as it can also create a ScriptContext and
(now)
> a
> > ModuleContext, not just a FunctionContext. Any suggestions?
> 
> BuildNewContext?

Could we do a rename pass on all the context creation functions to make them
consistent. How about:
 
 VisitNewLocalFunctionContext -> BuildNewLocalActivationContext
 VisitBuildLocalActivationContext -> BuildLocalActivationContextInitialization
 VisitNewLocalBlockContext -> BuildNewLocalBlockContext
 VisitNewLocalWithContext -> BuildNewLocalCatchContext
 VisitNewLocalCatchContext -> BuildNewLocalCatchContext

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3197:
DCHECK(Register::AreContiguous(module, closure, scope_info));
Please use PrepareForConsecutiveAllocations / NextConsecutiveRegister (also if
you could update the script scope case below that would be great, thanks).

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3203: .StoreAccumulatorInRegister(closure)
.MoveRegister(Register::function_closure(), closure)

(also below in script scope too please)

https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3207: } else if (scope->is_script_scope())
{
nit - could we do script scope first.

[neis, 2016-09-12 09:05:41]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 09:05:52]

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

[neis, 2016-09-12 09:23:27]

On 2016/09/05 11:36:07, rmcilroy wrote:
> Nice, Interpreter changes LGTM, didn't look at the rest.
> 
>
https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:748: VisitNewLocalFunctionContext();
> On 2016/09/02 17:51:51, adamk wrote:
> > On 2016/09/02 12:06:07, neis wrote:
> > > I'm not happy with this name, as it can also create a ScriptContext and
> (now)
> > a
> > > ModuleContext, not just a FunctionContext. Any suggestions?
> > 
> > BuildNewContext?
> 
> Could we do a rename pass on all the context creation functions to make them
> consistent. How about:
>  
>  VisitNewLocalFunctionContext -> BuildNewLocalActivationContext
>  VisitBuildLocalActivationContext -> BuildLocalActivationContextInitialization
>  VisitNewLocalBlockContext -> BuildNewLocalBlockContext
>  VisitNewLocalWithContext -> BuildNewLocalCatchContext
>  VisitNewLocalCatchContext -> BuildNewLocalCatchContext
> 
>
https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:3197:
> DCHECK(Register::AreContiguous(module, closure, scope_info));
> Please use PrepareForConsecutiveAllocations / NextConsecutiveRegister (also if
> you could update the script scope case below that would be great, thanks).
> 
>
https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:3203:
.StoreAccumulatorInRegister(closure)
> .MoveRegister(Register::function_closure(), closure)
> 
> (also below in script scope too please)
> 
>
https://codereview.chromium.org/2302783002/diff/140001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:3207: } else if
(scope->is_script_scope())
> {
> nit - could we do script scope first.

All done (the context functions naming in a separate CL).

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

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

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

Dry run: This issue passed the CQ dry run.

[neis, 2016-09-12 09:36:09]

The CQ bit was checked by neis@chromium.org

[neis, 2016-09-12 09:36:10]

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

[commit-bot: I haz the power, 2016-09-12 09:36:20]

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-09-12 09:39:49]

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

[commit-bot: I haz the power, 2016-09-12 09:39:50]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23789)

[ulan, 2016-09-12 10:46:46]

heap lgtm

[neis, 2016-09-12 10:48:39]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-09-12 10:48:52]

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-09-12 10:51:12]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-09-12 10:51:13]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-12 10:51:32]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

[commit-bot: I haz the power, 2016-09-12 10:51:33]

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

[neis, 2016-09-12 11:33:18]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2328283002/ by neis@chromium.org.

The reason for reverting is: Failures related to deopt..

[Michael Achenbach, 2016-09-12 11:38:42]

On 2016/09/12 11:33:18, neis wrote:
> A revert of this CL (patchset #10 id:180001) has been created in
> https://codereview.chromium.org/2328283002/ by mailto:neis@chromium.org.
> 
> The reason for reverting is: Failures related to deopt..

It reaches some unreachable code, e.g.:

out/Release/d8 --test --random-seed=-360320151 --deopt-every-n-times 1000000000
--print-deopt-stress --nohard-abort --nodead-code-elimination --nofold-constants
--noconcurrent-recompilation /b/swarm_slave/w/ir0gFr_i/test/mjsunit/mjsunit.js
--module /b/swarm_slave/w/ir0gFr_i/test/mjsunit/modules-exports2.js
=== mjsunit/modules-exports1 ===
--- stderr ---
#
# Fatal error in , line 0
# unreachable code
#

Some tests are simply not suitable for deopt fuzzing. If that's the case here,
you can add the tests in the deopt_fuzzer == True section in mjsunit.status.

[neis, 2016-09-12 12:20:03]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

[neis, 2016-09-12 12:20:21]

The CQ bit was checked by neis@chromium.org

[neis, 2016-09-12 12:20:21]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
adamk@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2302783002/#ps200001
(title: "Disable module tests for deopt fuzzer.")

[commit-bot: I haz the power, 2016-09-12 12:20:26]

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-09-12 12:54:52]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

[commit-bot: I haz the power, 2016-09-12 12:54:54]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-12 12:55:45]

Description was changed from

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Cr-Commit-Position: refs/heads/master@{#39341}
==========

to

==========
[modules] Basic support of exports

This adds partial support of exports to the runtime system and
to the interpreter. It introduces a new HeapObject JSModule that
maps each of the module's export names to a Cell containing the
exported value.

Several aspects of this implementation are subject to change in
follow-up CLs.

BUG=v8:1569

Committed: https://crrev.com/241a0412eed919395a2e163b30b9b66071ce5c17
Committed: https://crrev.com/21cb1105471e0b94039ba4037c17f2cb57cb2a8f
Cr-Original-Commit-Position: refs/heads/master@{#39341}
Cr-Commit-Position: refs/heads/master@{#39352}
==========

[commit-bot: I haz the power, 2016-09-12 12:55:46]

Patchset 11 (id:??) landed as
https://crrev.com/21cb1105471e0b94039ba4037c17f2cb57cb2a8f
Cr-Commit-Position: refs/heads/master@{#39352}

[adamk, 2016-09-14 17:32:22]

https://codereview.chromium.org/2302783002/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2302783002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:706: 
On 2016/09/02 12:06:07, neis wrote:
> Here we awkwardly declare the module parameter, which required several changes
> in other files to allow parameters in module scope.
> 
> Alternatively, we could keep 0 parameters in the module scope and instead
treat
> module scopes as a special case in CompilationInfo::num_parameters:
> 
> int CompilationInfo::num_parameters() const {
>   if (scope()->is_module_scope()) return 1;
>   return !IsStub() ? scope()->num_parameters() : parameter_count_;
> }
> 
> We don't need a declared parameter variable because we never use the variable
> itself. We directly access the stack slot via Parameter(1) at the beginning of
> the module code, and then immediately store the module object into the context
> extension slot for future access.
> 
> This alternative is still somewhat ugly but perhaps less ugly.

I feel like this might be a good question for someone on the compiler or
interpreter teams (maybe mstarzinger would have an opinion?). From the point of
view of the parser and scope code it's obviously cleaner.

[rmcilroy, 2016-09-15 09:19:06]

Interpreter changes LGTM, but could you add an example or two of module exports
to test-bytecode-generator / bytecode_expectations?