[modules] Assign cell indices at validation time.

[modules] Assign cell indices at validation time.

Instead of having a MODULE variable's index be 0 or 1, let it be the index of
its cell.  In this CL, we assign the indices but we continue to only use them to
distinguish imports from exports.  Actually using them to directly access the
cells will be done in a later CL.

R=adamk@chromium.org
BUG=v8:1569
Committed: https://crrev.com/1dd241c44bab3facea7abe5cb0e51e1315b376ce
Cr-Commit-Position: refs/heads/master@{#40752}

[Yang, 2016-10-31 12:57:04]

On 2016/10/31 11:12:02, neis wrote:

src/debug lgtm.

[adamk, 2016-10-31 18:25:04]

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc
File src/ast/modules.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode158
src/ast/modules.cc:158: for (int i = 0, length_i = data->length(); i <
length_i;) {
Either this should have a DCHECK that data->length() % 2 == 0, or the loop
should be bounded on i+1

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode213
src/ast/modules.cc:213: do {
Please add a comment for this loop, I expect it will be obscure to future code
archaeologists.

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode231
src/ast/modules.cc:231: DCHECK_EQ(entry->cell_index, 0);
This 0 now appears three times, perhaps it should have a constant that refers to
it?

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

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.h#newcode85
src/ast/modules.h:85: int cell_index;
Maybe add a comment here too, especially since 0 is a special value?

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.h#newcode197
src/ast/modules.h:197: void AssignCellIndices();
Please give this a comment: how are cell indices assigned?

https://codereview.chromium.org/2460233003/diff/1/src/ast/variables.h
File src/ast/variables.h (right):

https://codereview.chromium.org/2460233003/diff/1/src/ast/variables.h#newcode105
src/ast/variables.h:105: DCHECK_NE(index(), 0);
So this 0 appears in lots of places, but as I think more about it I'm not sure
it'd be any more clear to make it a constant. I do think this needs to be
clearly documented somewhere. Thoughts on where that might be?

https://codereview.chromium.org/2460233003/diff/1/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/debug/debug-scopes.cc#n...
src/debug/debug-scopes.cc:805: is_export = index > 0;
Comparisons like these getting added in various places also worry me. Perhaps
there's some way to encapsulate more of this logic? Not sure if that would be in
ModuleDescriptor or Variable. But It seems really fragile to have to have all
these magic zeros and >, < operators around (too easy to get one wrong and have
it slip through review).

https://codereview.chromium.org/2460233003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/objects.cc#newcode20016
src/objects.cc:20016: // TODO(neis): Make this more robust.
Related to robustness, seems like this loop should be bounded in a safer way.

https://codereview.chromium.org/2460233003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2460233003/diff/1/src/objects.h#newcode4550
src/objects.h:4550: // module variable with the given name (the index value of a
MODUULE variable
Typo: s/MODUULE/MODULE/

[neis, 2016-11-03 10:43:48]

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc
File src/ast/modules.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode158
src/ast/modules.cc:158: for (int i = 0, length_i = data->length(); i <
length_i;) {
On 2016/10/31 18:25:04, adamk wrote:
> Either this should have a DCHECK that data->length() % 2 == 0, or the loop
> should be bounded on i+1

Now it should be modulo 3. Yeah that's part of the TODO about making the regular
exports handling more robust. (Not sure what you mean by bounded on i+1.)

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode213
src/ast/modules.cc:213: do {
On 2016/10/31 18:25:04, adamk wrote:
> Please add a comment for this loop, I expect it will be obscure to future code
> archaeologists.

Done.

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.cc#newcode231
src/ast/modules.cc:231: DCHECK_EQ(entry->cell_index, 0);
On 2016/10/31 18:25:04, adamk wrote:
> This 0 now appears three times, perhaps it should have a constant that refers
to
> it?

I'd like to keep it just 0 for now but it's on my list of things to get back to.

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

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.h#newcode85
src/ast/modules.h:85: int cell_index;
On 2016/10/31 18:25:04, adamk wrote:
> Maybe add a comment here too, especially since 0 is a special value?

Done.

https://codereview.chromium.org/2460233003/diff/1/src/ast/modules.h#newcode197
src/ast/modules.h:197: void AssignCellIndices();
On 2016/10/31 18:25:04, adamk wrote:
> Please give this a comment: how are cell indices assigned?

Done.

https://codereview.chromium.org/2460233003/diff/1/src/ast/variables.h
File src/ast/variables.h (right):

https://codereview.chromium.org/2460233003/diff/1/src/ast/variables.h#newcode105
src/ast/variables.h:105: DCHECK_NE(index(), 0);
On 2016/10/31 18:25:04, adamk wrote:
> So this 0 appears in lots of places, but as I think more about it I'm not sure
> it'd be any more clear to make it a constant. I do think this needs to be
> clearly documented somewhere. Thoughts on where that might be?

There's a comment now in modules.h.

https://codereview.chromium.org/2460233003/diff/1/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/debug/debug-scopes.cc#n...
src/debug/debug-scopes.cc:805: is_export = index > 0;
On 2016/10/31 18:25:04, adamk wrote:
> Comparisons like these getting added in various places also worry me. Perhaps
> there's some way to encapsulate more of this logic? Not sure if that would be
in
> ModuleDescriptor or Variable. But It seems really fragile to have to have all
> these magic zeros and >, < operators around (too easy to get one wrong and
have
> it slip through review).

It's already in Variable (IsExport), but here we don't have a Variable. 
Fortunately, this one will go away in a followup change that replaces LoadImport
and LoadExport with a single LoadVariable.

https://codereview.chromium.org/2460233003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2460233003/diff/1/src/objects.cc#newcode20016
src/objects.cc:20016: // TODO(neis): Make this more robust.
On 2016/10/31 18:25:04, adamk wrote:
> Related to robustness, seems like this loop should be bounded in a safer way.

Sure, that's part of it.

https://codereview.chromium.org/2460233003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2460233003/diff/1/src/objects.h#newcode4550
src/objects.h:4550: // module variable with the given name (the index value of a
MODUULE variable
On 2016/10/31 18:25:04, adamk wrote:
> Typo: s/MODUULE/MODULE/

Done.

[neis, 2016-11-03 12:57:13]

Update: I tried to better encapsulate the cell_index business in
https://codereview.chromium.org/2465283004/.

[adamk, 2016-11-03 18:10:20]

lgtm % typo

https://codereview.chromium.org/2460233003/diff/40001/src/ast/modules.cc
File src/ast/modules.cc (right):

https://codereview.chromium.org/2460233003/diff/40001/src/ast/modules.cc#newc...
src/ast/modules.cc:213: // This local name may be exported under mulitple export
names.  Assign the
Typo: s/mulitple/multiple/

[neis, 2016-11-04 08:24:02]

The CQ bit was checked by neis@chromium.org

[neis, 2016-11-04 08:24:03]

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

[commit-bot: I haz the power, 2016-11-04 08:24:10]

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-11-04 08:51:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 22:22:07]

Description was changed from

==========
[modules] Assign cell indices at validation time.

Instead of having a MODULE variable's index be 0 or 1, let it be the index of
its cell.  In this CL, we assign the indices but we continue to only use them to
distinguish imports from exports.  Actually using them to directly access the
cells will be done in a later CL.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Assign cell indices at validation time.

Instead of having a MODULE variable's index be 0 or 1, let it be the index of
its cell.  In this CL, we assign the indices but we continue to only use them to
distinguish imports from exports.  Actually using them to directly access the
cells will be done in a later CL.

R=adamk@chromium.org
BUG=v8:1569

Committed: https://crrev.com/1dd241c44bab3facea7abe5cb0e51e1315b376ce
Cr-Commit-Position: refs/heads/master@{#40752}
==========

[commit-bot: I haz the power, 2016-11-17 22:22:09]

Patchset 4 (id:??) landed as
https://crrev.com/1dd241c44bab3facea7abe5cb0e51e1315b376ce
Cr-Commit-Position: refs/heads/master@{#40752}