wasm: generate bounds check the same way in debug/opt builds

RESOLVED FIXED in Firefox 52

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8809096 [details] [diff] [review]
wasm-check-bce.patch

Bug 1315596 wasn't triggering under a debug build because a redundant check was generated anyways and this perturbed the register allocation patterns. I think using debug vs opt mode shouldn't have an impact here; in particular, this might hide a few fuzz bugs.

Here's a patch offering a new option for the shell (--wasm-check-bce) as well as the env option (JITOPTIONS_wasmAlwaysCheckBounds) to trigger the old behavior.

It's enabled on the wasm/memory and wasm/bce tests; but really we could use it for all the tests by including it in the directives.txt file (thanks Lars!).
Attachment #8809096 - Flags: review?(luke)

Comment 1

2 years ago
Comment on attachment 8809096 [details] [diff] [review]
wasm-check-bce.patch

Review of attachment 8809096 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/jit-test/jit_test.py
@@ +200,5 @@
>      # wasm-baseline run when requesting --jitflags=interp, but the test
>      # contains test-also-noasmjs.)
>      test_flags = get_jitflags(options.jitflags)
>      options.can_test_also_noasmjs = True
> +    options.can_test_also_wasm = True

Perhaps instead we could name 'asmjs_disabled'/'wasm_disabled'?
Attachment #8809096 - Flags: review?(luke) → review+

Comment 2

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07bbb0e3dde4
Add an env option to always enable wasm bounds check generation; r=luke
Priority: -- → P3

Comment 3

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07bbb0e3dde4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.