[wasm] Do not use setjmp/longjmp in cctests.

[wasm] Do not use setjmp/longjmp in cctests.

The use of setjmp/longjmp makes the cctests in test-run-wasm and
test-run-wasm-64 flaky on Windows, and I think that it is better not
to use it. With this CL I replace it as follows:

Similar to the setjmp/longjmp implementation we still call a C
function when a trap happens. However, instead of calling longjmp in
this C function we just set a flag which indicates that a trap
happened and then return. After we return from the C function we leave
the frame of the current wasm function and return with a RET
instruction. At the end of a test the wasm test runner checks the flag
to see if a trap happened.

Please take a special look at the LeaveFrame function on arm64.

R=titzer@chromium.org, clemensh@chromium.org, v8-arm-ports@googlegroups.com
CC=jarin@chromium.org

Review-Url: https://codereview.chromium.org/2685583003
Cr-Commit-Position: refs/heads/master@{#43095}
Committed: https://chromium.googlesource.com/v8/v8/+/79570f87a1b467846a51c60186e4707f68a4d3d0

[ahaas, 2017-02-07 14:30:41]

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

[commit-bot: I haz the power, 2017-02-07 14:30:44]

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, 2017-02-07 14:49:08]

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

[commit-bot: I haz the power, 2017-02-07 14:49:09]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/20610)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/22253)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[ahaas, 2017-02-07 15:05:35]

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

[commit-bot: I haz the power, 2017-02-07 15:05:46]

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, 2017-02-07 15:25:35]

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

[commit-bot: I haz the power, 2017-02-07 15:25:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  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_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)

[ahaas, 2017-02-07 17:30:19]

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

[commit-bot: I haz the power, 2017-02-07 17:30:30]

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, 2017-02-07 18:00:09]

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

[commit-bot: I haz the power, 2017-02-07 18:00:10]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-02-07 18:14:59]

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

[commit-bot: I haz the power, 2017-02-07 18:15:02]

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, 2017-02-07 18:38:40]

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

[commit-bot: I haz the power, 2017-02-07 18:38:41]

Dry run: This issue passed the CQ dry run.

[Rodolph Perfetta (ARM), 2017-02-08 01:40:54]

rodolph.perfetta@arm.com changed reviewers:
+ rodolph.perfetta@arm.com

[Rodolph Perfetta (ARM), 2017-02-08 01:40:55]

The arm/arm64 parts look good. Only minor nits.

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2672: __ Push(lr, fp);
no need for __ as you are in the macro assembler. This code works because __ is
#define to nothing here. This was done to allow some macro to work in the macro
assembler and outside.

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2695: __ Mov(csp, fp);
ditto.

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-64.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-64.cc:1323: #if (V8_HOST_ARCH_IA32 ||
V8_HOST_ARCH_X64) && \
This test will only run on ia32 or x64 now, if you want to avoid simulators
only: #ifndef USE_SIMULATOR

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:1051: #if (V8_HOST_ARCH_IA32 ||
V8_HOST_ARCH_X64) && \
#ifndef USE_SIMULATOR

[ahaas, 2017-02-08 09:08:55]

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

[commit-bot: I haz the power, 2017-02-08 09:08:58]

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

[ahaas, 2017-02-08 09:11:46]

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2672: __ Push(lr, fp);
On 2017/02/08 at 01:40:55, Rodolph Perfetta (ARM) wrote:
> no need for __ as you are in the macro assembler. This code works because __
is #define to nothing here. This was done to allow some macro to work in the
macro assembler and outside.

Done.

https://codereview.chromium.org/2685583003/diff/60001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2695: __ Mov(csp, fp);
On 2017/02/08 at 01:40:55, Rodolph Perfetta (ARM) wrote:
> ditto.

Done.

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-64.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-64.cc:1323: #if (V8_HOST_ARCH_IA32 ||
V8_HOST_ARCH_X64) && \
On 2017/02/08 at 01:40:55, Rodolph Perfetta (ARM) wrote:
> This test will only run on ia32 or x64 now, if you want to avoid simulators
only: #ifndef USE_SIMULATOR

Thanks, I was not aware of this flag.

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2685583003/diff/60001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm.cc:1051: #if (V8_HOST_ARCH_IA32 ||
V8_HOST_ARCH_X64) && \
On 2017/02/08 at 01:40:55, Rodolph Perfetta (ARM) wrote:
> #ifndef USE_SIMULATOR

Done.

[commit-bot: I haz the power, 2017-02-08 09:32:09]

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

[commit-bot: I haz the power, 2017-02-08 09:32:09]

Dry run: This issue passed the CQ dry run.

[Rodolph Perfetta (ARM), 2017-02-08 20:50:36]

the arm/arm64 parts look fine.

[Clemens Hammacher, 2017-02-08 22:13:21]

This implementation is problematic if there is more than one wasm function on
the stack.
If we are limited to this case anyway, then can't we just return 0xdeadbeef
directly instead of this c-callback dance?
It would be nice of course to not change the code generation for cc tests, but
it looks like we are doing this already anyway...

https://codereview.chromium.org/2685583003/diff/80001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/2685583003/diff/80001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2693: if (type == StackFrame::WASM_COMPILED)
{
Couldn't you avoid the code duplication by something like
Register sp = type == StackFrame::WASM_COMPILED ? csp : jssp;

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:744: // This field has to be static.
Otherwise, gcc complains about the using in
Can you fix this typo ("using" should be "use" IMO) while you are here?

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:765: // Use setjmp/longjmp to deal with traps
in WebAssembly code.
This comment is obsolete now.

[ahaas, 2017-02-09 08:56:03]

On 2017/02/08 at 22:13:21, clemensh wrote:
> This implementation is problematic if there is more than one wasm function on
the stack.
> If we are limited to this case anyway, then can't we just return 0xdeadbeef
directly instead of this c-callback dance?
> It would be nice of course to not change the code generation for cc tests, but
it looks like we are doing this already anyway...

Agreed, if the return value of a trapping function were used by its caller, then
we would have a problem. However, I think this is an acceptable restriction for
these cctests.

I do prefer the c-callback to just returning 0xdeadbeef though, because
returning 0xdeadbeef would hard-code information into the code generator which I
think does not belong there. For example, the code generator does not know which
registers are the return registers. This information is stored in the call
descriptor, which is not available when we generate the code for trapif. In
addition we do not know the return type, so we would have to store 0xdeadbeef in
all possible return registers.

[ahaas, 2017-02-09 09:33:40]

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

[commit-bot: I haz the power, 2017-02-09 09:33:47]

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, 2017-02-09 09:57:16]

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

[commit-bot: I haz the power, 2017-02-09 09:57:18]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-02-09 16:59:37]

https://codereview.chromium.org/2685583003/diff/80001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/2685583003/diff/80001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.cc:2693: if (type == StackFrame::WASM_COMPILED)
{
On 2017/02/08 at 22:13:20, Clemens Hammacher wrote:
> Couldn't you avoid the code duplication by something like
> Register sp = type == StackFrame::WASM_COMPILED ? csp : jssp;

I implemented it this way because EnterFrame is constructed the same. I don't
think code sharing would be good here, because if this code changes, then most
likely it would change differently for csp and jssp.

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:744: // This field has to be static.
Otherwise, gcc complains about the using in
On 2017/02/08 at 22:13:20, Clemens Hammacher wrote:
> Can you fix this typo ("using" should be "use" IMO) while you are here?

Done.

https://codereview.chromium.org/2685583003/diff/80001/test/cctest/wasm/wasm-r...
test/cctest/wasm/wasm-run-utils.h:765: // Use setjmp/longjmp to deal with traps
in WebAssembly code.
On 2017/02/08 at 22:13:20, Clemens Hammacher wrote:
> This comment is obsolete now.

Done.

[Clemens Hammacher, 2017-02-10 12:17:57]

lgtm

[ahaas, 2017-02-10 12:48:31]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2017-02-10 12:48:42]

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, 2017-02-10 12:51:31]

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

[commit-bot: I haz the power, 2017-02-10 12:51:33]

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/34454)

[Jarin, 2017-02-10 12:56:00]

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

[Jarin, 2017-02-10 12:56:01]

lgtm

[ahaas, 2017-02-10 12:56:24]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2017-02-10 12:56:39]

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, 2017-02-10 13:11:45]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486731384550920,
"parent_rev": "e03a220611bcff9ed294bcc940bbb5b0c6f7f386", "commit_rev":
"79570f87a1b467846a51c60186e4707f68a4d3d0"}

[commit-bot: I haz the power, 2017-02-10 13:11:50]

Description was changed from

==========
[wasm] Do not use setjmp/longjmp in cctests.

The use of setjmp/longjmp makes the cctests in test-run-wasm and
test-run-wasm-64 flaky on Windows, and I think that it is better not
to use it. With this CL I replace it as follows:

Similar to the setjmp/longjmp implementation we still call a C
function when a trap happens. However, instead of calling longjmp in
this C function we just set a flag which indicates that a trap
happened and then return. After we return from the C function we leave
the frame of the current wasm function and return with a RET
instruction. At the end of a test the wasm test runner checks the flag
to see if a trap happened.

Please take a special look at the LeaveFrame function on arm64.

R=titzer@chromium.org, clemensh@chromium.org, v8-arm-ports@googlegroups.com
CC=jarin@chromium.org
==========

to

==========
[wasm] Do not use setjmp/longjmp in cctests.

The use of setjmp/longjmp makes the cctests in test-run-wasm and
test-run-wasm-64 flaky on Windows, and I think that it is better not
to use it. With this CL I replace it as follows:

Similar to the setjmp/longjmp implementation we still call a C
function when a trap happens. However, instead of calling longjmp in
this C function we just set a flag which indicates that a trap
happened and then return. After we return from the C function we leave
the frame of the current wasm function and return with a RET
instruction. At the end of a test the wasm test runner checks the flag
to see if a trap happened.

Please take a special look at the LeaveFrame function on arm64.

R=titzer@chromium.org, clemensh@chromium.org, v8-arm-ports@googlegroups.com
CC=jarin@chromium.org

Review-Url: https://codereview.chromium.org/2685583003
Cr-Commit-Position: refs/heads/master@{#43095}
Committed:
https://chromium.googlesource.com/v8/v8/+/79570f87a1b467846a51c60186e4707f68a...
==========

[commit-bot: I haz the power, 2017-02-10 13:11:51]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/79570f87a1b467846a51c60186e4707f68a...