[wasm] Support validation of asm.js modules with != 3 args.

[wasm] Support validation of asm.js modules with != 3 args.

Our previous per-arch instantiation thunks for asm.js
didn't support modules that had or were called with anything other
than 3 arguments. Adding support for this.

Addding a runtime test method to check if asm validation succeeded.

Adding a test of validation with different argument count combinations.

R=mstarzinger@chromium.org
TEST=mjsunit/asm/asm-validator.js
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203

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

[bradnelson, 2016-08-09 08:02:07]

Michael,

This is not quite ready (only works on x86), but I'm hoping to get your feedback
on a few points:
* What's a good place to consult to select scratch registers for these builtins?
* Is this approach to peeling back the stack an extra frame ok?
* The simulator for arm seems to make some assumptions about what can happen
with stack accesses. Are the simulators likely to get tripped up by dropping the
parent frame in the success case?
* I've noticed issues with going all the way back to CompileLazy when the
argument counts vary, CompileBaseline seems to work. Any guess what's going on?

Thanks!

[bradnelson, 2016-08-09 08:03:25]

Oh and is allowing explicit -1 for user specified argument count an ok way to
allow the caller to pick the count?

[Michael Starzinger, 2016-08-09 15:48:09]

https://codereview.chromium.org/2229723002/diff/20001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2229723002/diff/20001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1112: __
CallRuntime(Runtime::kInstantiateAsmJs, -1);
For the call-site to not pass the arguments count is currently not supported.
And I don't think we want to go down that path. Even if the implementation of
the runtime function has var-args semantics, the call-site should always
statically know the argument count. See my comment in runtime.h to avoid this
issue completely.

https://codereview.chromium.org/2229723002/diff/20001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2229723002/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:133: F(InstantiateAsmJs, -1, 1)
I would very much vote for keeping the arguments count to this runtime function
fixed to 4 and instead pass sentinel values (e.g. Smi(0) or undefined) when the
builtin cannot provide the values. The implementation of the runtime function is
already resilient enough to deal with arbitrary values for arguments 1, 2 and 3
AFAICT.

[Michael Starzinger, 2016-08-09 16:42:06]

Trying to answer inline ...

On 2016/08/09 08:02:07, bradnelson wrote:
> This is not quite ready (only works on x86), but I'm hoping to get your
feedback
> on a few points:
> * What's a good place to consult to select scratch registers for these
builtins?

Unfortunately only the surrounding code can tell. Not all architectures have
been given a scratch register.

> * Is this approach to peeling back the stack an extra frame ok?

The dynamic removal of arguments in the success case should be doable. Not sure
what you mean by "extra frame", I don't see any extra frame being peeled
anywhere in the implementation of this CL (at least on x64, didn't check the
other architectures).

> * The simulator for arm seems to make some assumptions about what can happen
> with stack accesses. Are the simulators likely to get tripped up by dropping
the
> parent frame in the success case?

Again, not quite sure what you mean by "dropping the parent frame". The only
thing I can make out that is being dropped are the arguments to the module
instantiation function. And that is in line with our ABI, the simulators are
fine with that. We have other places where we change the arguments stored
"underneath" the return address. See usages of
MacroAssembler::DropUnderReturnAddress or MacroAssembler::PopReturnAddressTo (or
other places where we do it explicitly) for example.

> * I've noticed issues with going all the way back to CompileLazy when the
> argument counts vary, CompileBaseline seems to work. Any guess what's going
on?

This shouldn't be the case, CompileLazy is the right thing and the interface of
those two runtime function shouldn't vary all to much. We should definitely go
back to CompileLazy and get that working.

> Oh and is allowing explicit -1 for user specified argument count an ok way to
> allow the caller to pick the count?

As one of my comment says, I would very much try to avoid that. The changes to
the MacroAssembler shouldn't be needed. We should just makes
Runtime_InstantiateAsmJs always take four arguments fixed and make the builtin
act as an arguments adapter that pads missing values with "undefined" (or
another sentinel). AFAICT there is no semantic different between Module(stdlib,
undefined) and Module(stdlib). That is also very much in sync with how
JavaScript behaves in general.

[Michael Starzinger, 2016-08-09 17:20:31]

On 2016/08/09 08:02:07, bradnelson wrote:
> * I've noticed issues with going all the way back to CompileLazy when the
> argument counts vary, CompileBaseline seems to work. Any guess what's going
on?

I have a theory as to what might go wrong. When we call into CompileLazy from
the instantiation builtin, JSFunction::code as well as SharedFunctionInfo::code
is still pointing to the instantiation builtin. This makes both look as if
though they are compiled. This means JSFunction::is_compiled will return true
and the compiler will happily return without doing anything.

This might sound trivial to fix, but it is somewhat intricate if my theory is
correct. I'll need to dwell a bit about that.

[Michael Starzinger, 2016-08-09 17:45:01]

On 2016/08/09 17:20:31, Michael Starzinger wrote:
> On 2016/08/09 08:02:07, bradnelson wrote:
> > * I've noticed issues with going all the way back to CompileLazy when the
> > argument counts vary, CompileBaseline seems to work. Any guess what's going
> on?
> 
> I have a theory as to what might go wrong. When we call into CompileLazy from
> the instantiation builtin, JSFunction::code as well as
SharedFunctionInfo::code
> is still pointing to the instantiation builtin. This makes both look as if
> though they are compiled. This means JSFunction::is_compiled will return true
> and the compiler will happily return without doing anything.
> 
> This might sound trivial to fix, but it is somewhat intricate if my theory is
> correct. I'll need to dwell a bit about that.

Yes, my theory is correct, already on tip-of-tree the following causes an
endless loop because the fall-back case just plainly doesn't work and is
untested:

$ ./out/x64.debug/d8 -e "function Module(a,b,c) { 'use asm'; return {} }
Module(1,2,3)" --validate
-asm

Can we start testing this more thoroughly?

[bradnelson, 2016-08-11 09:16:39]

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

[commit-bot: I haz the power, 2016-08-11 09:16:42]

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

[bradn, 2016-08-11 09:19:32]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-08-11 09:19:33]

Indeed you are right about the state of the fallback not working in master.

I had done a bunch of manual testing before I'd switched CompileBaseline to
CompileLazy during review, but didn't do a good a job rechecking before landing.
:-(

Of course what I should have done was have automated tests, which hopefully the
runtime method I added in this change does ok.

In any event, as you also predicted is_compiled() is guiding things through a
tricky path.
Once I get rid of the early outs in that path though, I ended up needing to keep
the AsmWasmData set longer to mark failure. Does this seem ok, or is there a
better place to stash a flag to indicate a failed asm->wasm attempt?

PTAL

https://codereview.chromium.org/2229723002/diff/20001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2229723002/diff/20001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1112: __
CallRuntime(Runtime::kInstantiateAsmJs, -1);
On 2016/08/09 15:48:08, Michael Starzinger wrote:
> For the call-site to not pass the arguments count is currently not supported.
> And I don't think we want to go down that path. Even if the implementation of
> the runtime function has var-args semantics, the call-site should always
> statically know the argument count. See my comment in runtime.h to avoid this
> issue completely.

Ah, great idea!
I like that much better.
Done.

https://codereview.chromium.org/2229723002/diff/20001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2229723002/diff/20001/src/runtime/runtime.h#n...
src/runtime/runtime.h:133: F(InstantiateAsmJs, -1, 1)
On 2016/08/09 15:48:09, Michael Starzinger wrote:
> I would very much vote for keeping the arguments count to this runtime
function
> fixed to 4 and instead pass sentinel values (e.g. Smi(0) or undefined) when
the
> builtin cannot provide the values. The implementation of the runtime function
is
> already resilient enough to deal with arbitrary values for arguments 1, 2 and
3
> AFAICT.

Indeed.
Done.

[commit-bot: I haz the power, 2016-08-11 09:20:46]

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

[commit-bot: I haz the power, 2016-08-11 09:20:47]

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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10802)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_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...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[bradnelson, 2016-08-11 09:24:45]

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

[commit-bot: I haz the power, 2016-08-11 09:24:49]

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-08-11 09:38:22]

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

[commit-bot: I haz the power, 2016-08-11 09:38:23]

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

[Michael Starzinger, 2016-08-11 09:44:25]

Quick reply to questions inline. Will do a second review of the code later.

On 2016/08/11 09:19:33, bradn wrote:
> In any event, as you also predicted is_compiled() is guiding things through a
> tricky path.
> Once I get rid of the early outs in that path though, I ended up needing to
keep
> the AsmWasmData set longer to mark failure. Does this seem ok, or is there a
> better place to stash a flag to indicate a failed asm->wasm attempt?

AFAICT there should still be bits available in the compiler hints (i.e. the
{SharedFunctionInfo::CompilerHints} enum). We could add a "kAsmWasmBroken" bit
(better name appreciated) there that gets set whenever the dynamic validation of
module instantiation parameters fails. This will make sure we don't end in an
endless loop when we recompile. Then we should be able to clear the WASM data.

Also about the {is_compiled} check: I would vote for resetting
{JSFunction::code} as well as {SharedFunctionInfo::code} back to the lazy
compile stub when the instantiation fails. That way no special casing is
required anywhere in the compilation pipeline. If we take care to only do this
resetting when the respective fields actually point to the instantiation
builtin, then this should be safe.

[Michael Starzinger, 2016-08-11 11:44:10]

I like where this is going. Already looking good. One more round of comments.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1078: for (int j = 0; j < 4; ++j) {
Does this work correctly in the case where we instantiate the module with more
than 3 arguments? Can we add a test case for that. I am specifically thinking
about a case where the module has more than three formal parameters, like the
following:

function Module(stdlib, foreign, heap, crap1, crap2) {
  'use asm';
  return {};
}
var m = Module({},{},heap);

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1104: __ Pop(kScratchRegister);
nit: Let's use __ Drop(2) instead of the multi-pop. Probably applies to all
archs.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1109: __ Pop(rbx);
nit: Lets use PopReturnAddressTo for readability.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1112: __ Push(rbx);
nit: Lets use PushReturnAddressFrom for readability.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode480
src/compiler.cc:480: !info->shared_info()->HasAsmWasmData()) {
As mentioned in my previous comment, we could go with a bit in the
{CompilationHints} of the {SharedFunctionInfo} to indicate instantiation
failures. Then the clearing itself can be done in Runtime_InstantiateAsmJs as
before.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1018: function->GetIsolate()->builtins()->builtin(
This can be avoided by adding the following to the failure case in the
Runtime_InstantiateAsmJs function:

DCHECK(function->code() ==
isolate->builtins()->builtin(Builtins::kInstantiateAsmJs));
function->ReplaceCode(isolate->builtins()->builtin(Builtins::kCompileLazy));

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1041: if (function->shared()->is_compiled() &&
This can be avoided by adding the following to the failure case in the
Runtime_InstantiateAsmJs function:

if (shared->code() == isolate->builtins()->builtin(Builtins::kInstantiateAsmJs))
{
  shared->ReplaceCode(isolate->builtins()->builtin(Builtins::kCompileLazy));
}

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1224: if (function->is_compiled() &&
Likewise.

https://codereview.chromium.org/2229723002/diff/60001/src/runtime/runtime-com...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/runtime/runtime-com...
src/runtime/runtime-compiler.cc:105: // AsmWasmData will get removed later as a
consequence (but is kept so we
See comments in compiler.cc about this.

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
File test/mjsunit/asm/asm-validation.js (right):

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
test/mjsunit/asm/asm-validation.js:3: // found in the LICENSE file.
Can we add tests for the following?

- Instantiating module via constructor call (i.e. var m = new Module()).
- Having multiple closures for one module, failing in sequence:
    function MkModule() {
      function Module(stdlib) { 'use asm'; return {} }
      return Module;
    }
    var Module1 = MkModule();
    var Module2 = MkModule();
    var m1 = Module(1,2,3);
    var m2 = Module(4,5,6);
- Simiar to the above where the second instantiation would actually succeed:
    var m1 = Module(1,2,3);
    var m2 = Module({},{},heap);
- Creating bound function from module
    var ModuleBound = Module.bind(this, stdlib, {}, heap);
    var m = ModuleBound();

[bradnelson, 2016-08-12 00:27:33]

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

[commit-bot: I haz the power, 2016-08-12 00:27:40]

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-08-12 00:33:02]

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

[commit-bot: I haz the power, 2016-08-12 00:33:03]

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

[bradnelson, 2016-08-12 01:14:27]

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

[commit-bot: I haz the power, 2016-08-12 01:14:37]

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

[bradn, 2016-08-12 01:17:09]

One issue I'm running into is that in stress test mode, since modules are
getting cached. Once they fail the first time round, they don't work as expected
in the second run.

Does this mean I should be trying to make them retry in certain circumstances /
have separate SharedFunctionInfos ?

Or is this an issue with testing for the state of the module explicitly with
%IsAsmWasmCode ?

Thanks for all your help with this change!

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1078: for (int j = 0; j < 4; ++j) {
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> Does this work correctly in the case where we instantiate the module with more
> than 3 arguments? Can we add a test case for that. I am specifically thinking
> about a case where the module has more than three formal parameters, like the
> following:
> 
> function Module(stdlib, foreign, heap, crap1, crap2) {
>   'use asm';
>   return {};
> }
> var m = Module({},{},heap);

The validator rejects the module above as invalid and falls back.
Added a test and that seems to work.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1104: __ Pop(kScratchRegister);
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> nit: Let's use __ Drop(2) instead of the multi-pop. Probably applies to all
> archs.

Good idea. Done.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1109: __ Pop(rbx);
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> nit: Lets use PopReturnAddressTo for readability.

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:1112: __ Push(rbx);
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> nit: Lets use PushReturnAddressFrom for readability.

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode480
src/compiler.cc:480: !info->shared_info()->HasAsmWasmData()) {
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> As mentioned in my previous comment, we could go with a bit in the
> {CompilationHints} of the {SharedFunctionInfo} to indicate instantiation
> failures. Then the clearing itself can be done in Runtime_InstantiateAsmJs as
> before.

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1018: function->GetIsolate()->builtins()->builtin(
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> This can be avoided by adding the following to the failure case in the
> Runtime_InstantiateAsmJs function:
> 
> DCHECK(function->code() ==
> isolate->builtins()->builtin(Builtins::kInstantiateAsmJs));
> function->ReplaceCode(isolate->builtins()->builtin(Builtins::kCompileLazy));

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1041: if (function->shared()->is_compiled() &&
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> This can be avoided by adding the following to the failure case in the
> Runtime_InstantiateAsmJs function:
> 
> if (shared->code() ==
isolate->builtins()->builtin(Builtins::kInstantiateAsmJs))
> {
>   shared->ReplaceCode(isolate->builtins()->builtin(Builtins::kCompileLazy));
> }

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1224: if (function->is_compiled() &&
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/2229723002/diff/60001/src/runtime/runtime-com...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/60001/src/runtime/runtime-com...
src/runtime/runtime-compiler.cc:105: // AsmWasmData will get removed later as a
consequence (but is kept so we
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> See comments in compiler.cc about this.

Done.

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
File test/mjsunit/asm/asm-validation.js (right):

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
test/mjsunit/asm/asm-validation.js:3: // found in the LICENSE file.
On 2016/08/11 11:44:10, Michael Starzinger wrote:
> Can we add tests for the following?
> 
> - Instantiating module via constructor call (i.e. var m = new Module()).
> - Having multiple closures for one module, failing in sequence:
>     function MkModule() {
>       function Module(stdlib) { 'use asm'; return {} }
>       return Module;
>     }
>     var Module1 = MkModule();
>     var Module2 = MkModule();
>     var m1 = Module(1,2,3);
>     var m2 = Module(4,5,6);
> - Simiar to the above where the second instantiation would actually succeed:
>     var m1 = Module(1,2,3);
>     var m2 = Module({},{},heap);
> - Creating bound function from module
>     var ModuleBound = Module.bind(this, stdlib, {}, heap);
>     var m = ModuleBound();

Done, but Questions:

* The new case seems to be able to use the wasm path. Is this what you expected?
* Failure then, success does not go down the wasm path, as the stays in the
non-wasm path once there is a failure. This is probably ok for now as this is a
fallback. But, am I correct in assuming that I would need to create a separate
SharedFunctionInfo for each failed function to separate them, or is there some
why I can separate them and have a different code object on each?

[commit-bot: I haz the power, 2016-08-12 01:18:26]

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

[commit-bot: I haz the power, 2016-08-12 01:18:27]

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

[Michael Starzinger, 2016-08-12 08:51:24]

Answers to questions inline. Will do a final review round over the code later
today.

On 2016/08/12 01:17:09, bradn wrote:
> One issue I'm running into is that in stress test mode, since modules are
> getting cached. Once they fail the first time round, they don't work as
expected
> in the second run.

I don't think the problem you are seeing is caused by the stress mode directly.
I think it is the --always-opt flag that is causing the problems. In the
%IsAsmWasmCode predicate we currently check {JSFunction::code} which might point
to an optimized version of the code (that's what --always-opt is forcing). I
think we should instead just check {SharedFunctionInfo::code} there, which is
always the unoptimized code, hence will remain to to be the instantiation
builtin as long as the module hasn't been destroyed.

> Does this mean I should be trying to make them retry in certain circumstances
/
> have separate SharedFunctionInfos ?

Retrying in certain circumstance will lead us into a world of pain. Because if
old activations of the FullCodegen version of the module are still on the stack,
mocking with the function object becomes really dangerous. I think we should
stick with the current approach where we destroy the WASM module immediately and
irreversibly as long as possible.

> Or is this an issue with testing for the state of the module explicitly with
> %IsAsmWasmCode ?

See comment above, I think a minor tweak to the predicate will fix the problem.

[Michael Starzinger, 2016-08-12 09:21:56]

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
File test/mjsunit/asm/asm-validation.js (right):

https://codereview.chromium.org/2229723002/diff/60001/test/mjsunit/asm/asm-va...
test/mjsunit/asm/asm-validation.js:3: // found in the LICENSE file.
On 2016/08/12 01:17:09, bradn wrote:
> * The new case seems to be able to use the wasm path. Is this what you
expected?

Yes, that is what my intuition of the asm.js spec was.

> * Failure then, success does not go down the wasm path, as the stays in the
> non-wasm path once there is a failure. This is probably ok for now as this is
a
> fallback. But, am I correct in assuming that I would need to create a separate
> SharedFunctionInfo for each failed function to separate them, or is there some
> why I can separate them and have a different code object on each?

We cannot create a separate SharedFunctionInfo, that is out of the question. The
current approach will destroy the module the moment is instantiated with bogus
parameters and from that point on the module is broken and can no longer go down
the WASM path. IIRC we discussed this a couple of months back and the conclusion
was that this would be fine. If we want to "revive" a broken module or have the
possibility to have a module be both, compiled by WASM and FullCodegen at the
same time, then we need to rethink the entire design. With the current approach
this is not possible.

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:111: function->shared()->ReplaceCode(
Replacing the code on the SharedFunctionInfo is dangerous, we cannot guarantee
that it hasn't already been re-compiled by FullCodegen before due to another
closure of the same module failing. We would regenerate the full-codegen code
and could have activations of both the old and the new version on the stack. Can
we make sure to only replace the shared code if it still is the instantiation
builtin?

if (function->shared()->code() ==
isolate->builtins()->builtin(Builtins::kInstantiateAsmJs)) {
  function->shared()->ReplaceCode(...);
}

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-te...
File src/runtime/runtime-test.cc (right):

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-te...
src/runtime/runtime-test.cc:634: if (function->code() !=
Lets just test function->shared()->code() here. The shared code is always the
unoptimized code and will remain to be the instantiation stub even when we
decide to optimize the module function. This will also cover the case where
--always-opt triggers optimization of the module code.

https://codereview.chromium.org/2229723002/diff/100001/test/mjsunit/asm/asm-v...
File test/mjsunit/asm/asm-validation.js (right):

https://codereview.chromium.org/2229723002/diff/100001/test/mjsunit/asm/asm-v...
test/mjsunit/asm/asm-validation.js:134: function Module(stdlib, ffi, heap) {
This is not exactly what I had in mind. This will yield one {JSFunction} object
representing the module. I was aiming at having multiple {JSFunction} objects
for a single piece of source code (i.e. for a single {SharedFunctionInfo}
obhect). This can be achieved with the snippet I posted in my last comment,
where the same function declaration is evaluated multiple times.

(function TestFailureThenSuccess() {
  function MkModule() {
    function Module(stdlib, ffi, heap) {
      "use asm";
      function foo() { return 123; }
      return { foo: foo };
    }
    return Module
  }
  var Module1 = MkModule();
  var Module2 = MkModule();
  var heap = new ArrayBuffer(1024 * 1024);
  var m1 = Module1(1, 2, 3);
  assertFalse(%IsAsmWasmCode(Module));
  var m2 = Module2({}, {}, heap);
  assertFalse(%IsAsmWasmCode(Module));
  assertEquals(123, m1.foo());
  assertEquals(123, m2.foo());
})();

Likewise for the previous test case above and the next test case below.

[bradnelson, 2016-08-12 17:03:48]

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

[commit-bot: I haz the power, 2016-08-12 17:03:57]

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-08-12 17:09:07]

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

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

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

[bradnelson, 2016-08-12 18:49:33]

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

[commit-bot: I haz the power, 2016-08-12 18:49:40]

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

[bradn, 2016-08-12 18:50:02]

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:111: function->shared()->ReplaceCode(
On 2016/08/12 09:21:55, Michael Starzinger wrote:
> Replacing the code on the SharedFunctionInfo is dangerous, we cannot guarantee
> that it hasn't already been re-compiled by FullCodegen before due to another
> closure of the same module failing. We would regenerate the full-codegen code
> and could have activations of both the old and the new version on the stack.
Can
> we make sure to only replace the shared code if it still is the instantiation
> builtin?
> 
> if (function->shared()->code() ==
> isolate->builtins()->builtin(Builtins::kInstantiateAsmJs)) {
>   function->shared()->ReplaceCode(...);
> }

Done.

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-te...
File src/runtime/runtime-test.cc (right):

https://codereview.chromium.org/2229723002/diff/100001/src/runtime/runtime-te...
src/runtime/runtime-test.cc:634: if (function->code() !=
On 2016/08/12 09:21:56, Michael Starzinger wrote:
> Lets just test function->shared()->code() here. The shared code is always the
> unoptimized code and will remain to be the instantiation stub even when we
> decide to optimize the module function. This will also cover the case where
> --always-opt triggers optimization of the module code.

Done.

https://codereview.chromium.org/2229723002/diff/100001/test/mjsunit/asm/asm-v...
File test/mjsunit/asm/asm-validation.js (right):

https://codereview.chromium.org/2229723002/diff/100001/test/mjsunit/asm/asm-v...
test/mjsunit/asm/asm-validation.js:134: function Module(stdlib, ffi, heap) {
On 2016/08/12 09:21:56, Michael Starzinger wrote:
> This is not exactly what I had in mind. This will yield one {JSFunction}
object
> representing the module. I was aiming at having multiple {JSFunction} objects
> for a single piece of source code (i.e. for a single {SharedFunctionInfo}
> obhect). This can be achieved with the snippet I posted in my last comment,
> where the same function declaration is evaluated multiple times.
> 
> (function TestFailureThenSuccess() {
>   function MkModule() {
>     function Module(stdlib, ffi, heap) {
>       "use asm";
>       function foo() { return 123; }
>       return { foo: foo };
>     }
>     return Module
>   }
>   var Module1 = MkModule();
>   var Module2 = MkModule();
>   var heap = new ArrayBuffer(1024 * 1024);
>   var m1 = Module1(1, 2, 3);
>   assertFalse(%IsAsmWasmCode(Module));
>   var m2 = Module2({}, {}, heap);
>   assertFalse(%IsAsmWasmCode(Module));
>   assertEquals(123, m1.foo());
>   assertEquals(123, m2.foo());
> })();
> 
> Likewise for the previous test case above and the next test case below.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-08-16 15:59:37]

LGTM.

[bradnelson, 2016-08-17 16:58:18]

The CQ bit was checked by bradnelson@chromium.org

[commit-bot: I haz the power, 2016-08-17 16:58:24]

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-08-17 17:22:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-17 17:22:38]

Description was changed from

==========
[wasm] Support validation of asm.js modules with != 3 args.

Our previous per-arch instantiation thunks for asm.js
didn't support modules that had or were called with anything other
than 3 arguments. Adding support for this.

Addding a runtime test method to check if asm validation succeeded.

Adding a test of validation with different argument count combinations.

R=mstarzinger@chromium.org
TEST=mjsunit/asm/asm-validator.js
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
==========

to

==========
[wasm] Support validation of asm.js modules with != 3 args.

Our previous per-arch instantiation thunks for asm.js
didn't support modules that had or were called with anything other
than 3 arguments. Adding support for this.

Addding a runtime test method to check if asm validation succeeded.

Adding a test of validation with different argument count combinations.

R=mstarzinger@chromium.org
TEST=mjsunit/asm/asm-validator.js
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203

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

[commit-bot: I haz the power, 2016-08-17 17:22:40]

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