[wasm] Fix compiled-module-management lifetime issues.

[wasm] Fix compiled-module-management lifetime issues.

This makes sure the test in question does not rely on specific lifetime
characteristics for local variables within a function. Note that these
lifetimes are not specified by JavaScript and are not observable within
JavaScript proper. The natives syntax however makes it observable.

BUG=v8:5345
TEST=mjsunit/wasm/compiled-module-management
R=mtrofin@chromium.org

Committed: https://crrev.com/e637154b8a9884bb627cdaaf3c356c3c1f8324d0
Cr-Commit-Position: refs/heads/master@{#40733}

[Michael Starzinger, 2016-11-03 14:05:38]

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

[commit-bot: I haz the power, 2016-11-03 14:05:42]

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-11-03 14:28:17]

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

[commit-bot: I haz the power, 2016-11-03 14:28:18]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-11-03 14:33:28]

On 2016/11/03 14:06:51, Michael Starzinger wrote:

Thanks for looking at this!

Does the approach in the CL preclude doing it all in a function? I'm assuming
so, since I gather the issue before was that the module instances were locals.
If so, perhaps we should leave the js file dedicated to this one test - could
you add a comment to that effect?

Now, the purpose of the test was to validate the finalization logic for module
instances, so it needs to rely on some sort of lifetime guarantees - which is
artificial, of course. The new approach relies on such guarantees, too, just not
for local variables in a function. I'm probably missing something, but I'm
trying to understand the benefit of this  CL - do we believe we can rely on
lifetime of global variables with more confidence than for local variables? If
not, perhaps the whole approach of testing this in a mjsunit is unsuitable for
the stated goal. If that is the case, what would the alternatives be?

[Michael Starzinger, 2016-11-03 14:55:33]

On 2016/11/03 14:33:28, Mircea Trofin wrote:
> On 2016/11/03 14:06:51, Michael Starzinger wrote:
> 
> Thanks for looking at this!
> 
> Does the approach in the CL preclude doing it all in a function? I'm assuming
> so, since I gather the issue before was that the module instances were locals.
> If so, perhaps we should leave the js file dedicated to this one test - could
> you add a comment to that effect?

I have added a comment above the global variables in that respect.

> Now, the purpose of the test was to validate the finalization logic for module
> instances, so it needs to rely on some sort of lifetime guarantees - which is
> artificial, of course. The new approach relies on such guarantees, too, just
not
> for local variables in a function. I'm probably missing something, but I'm
> trying to understand the benefit of this  CL - do we believe we can rely on
> lifetime of global variables with more confidence than for local variables? If
> not, perhaps the whole approach of testing this in a mjsunit is unsuitable for
> the stated goal. If that is the case, what would the alternatives be?

The semantics of global variables are fundamentally different in JavaScript
because they are properties on the global object, could be mutated by any call,
and hence are much harder to optimize without speculative assumptions. So at
least with Ignition we should be safe by using global variables.

Yes, a cctest would give much finer control over variable lifetime. Feel free to
move this mjsunit test to a cctest at your discretion. I would still like to
land this fix because I don't think it makes sense to have a test in the tree
that is disabled in all but one variant for months and fools us into thinking we
have test coverage for something when in fact we don't.

[Mircea Trofin, 2016-11-03 15:06:54]

lgtm

[Michael Starzinger, 2016-11-03 15:27:10]

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

[commit-bot: I haz the power, 2016-11-03 15:27:20]

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-11-03 15:50:58]

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

[commit-bot: I haz the power, 2016-11-03 15:50:58]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-11-03 15:51:20]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2016-11-03 15:51: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-11-03 15:53:23]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
[wasm] Fix compiled-module-management lifetime issues.

This makes sure the test in question does not rely on specific lifetime
characteristics for local variables within a function. Note that these
lifetimes are not specified by JavaScript and are not observable within
JavaScript proper. The natives syntax however makes it observable.

BUG=v8:5345
TEST=mjsunit/wasm/compiled-module-management
R=mtrofin@chromium.org
==========

to

==========
[wasm] Fix compiled-module-management lifetime issues.

This makes sure the test in question does not rely on specific lifetime
characteristics for local variables within a function. Note that these
lifetimes are not specified by JavaScript and are not observable within
JavaScript proper. The natives syntax however makes it observable.

BUG=v8:5345
TEST=mjsunit/wasm/compiled-module-management
R=mtrofin@chromium.org

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

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

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