Closed
Bug 1346076
Opened 8 years ago
Closed 6 years ago
[wasm] Differential Testing: Different output message involving wasm
Categories
(Core :: JavaScript: WebAssembly, defect)
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: gkw, Unassigned)
References
Details
(Keywords: testcase)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
print((function () { return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(` (module (func (export "f0") (result f32) i32.const 0 i32.const 0 i32.const 0 select i32.const 0 i32.const 0 select f32.convert_u/i32 ) (func (export "f1") i32.const 0 i32.const 0 i32.const 0 select i32.const 0 i32.const 0 i32.const 0 i32.const 0 select i32.const 0 select i32.const 0 i32.const 0 i32.const 0 select i32.const 0 i32.const 0 i32.const 0 select i32.const 0 select drop drop drop ) (func (export "f2") (result i32) f32.const nan:0x1 call 1 i32.reinterpret/f32 ) ) `))); })().exports.f2()); $ ./js-dbg-64-profDisabled-dm-linux-35398cae65c1 --fuzzing-safe --no-threads --ion-eager testcase.js 2143289344 $ ./js-dbg-64-profDisabled-dm-linux-35398cae65c1 --fuzzing-safe --no-threads --ion-eager --wasm-always-baseline testcase.js 2139095041 Tested this on m-c rev 35398cae65c1. My configure flags are: AR=ar sh ./configure --enable-debug --disable-profiling --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 35398cae65c1 autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/958074f3b830 user: Dan Gohman date: Fri Sep 23 09:13:15 2016 -0500 summary: Bug 1287220 - Baldr: update to binary version 0xc (r=luke) Not sure if this is directly related. Benjamin, is bug 1287220 a likely regressor?
Flags: needinfo?(bbouvier)
![]() |
Reporter | |
Comment 1•8 years ago
|
||
The supposed regressing changeset introduced drops, which are present in the testcase, so I guess that's why it's related. Let me know if the testcase can be reduced further.
Comment 2•8 years ago
|
||
The discrepancy is a result of --enable-more-deterministic, which causes the '1' to be stripped from the NaN at some point. That configuration flag is ignored by the baseline compiler. (You see this more easily if you print the result as hex.) The bodies of f0 and f1 can be empty, and you get the same result. All that's required is that f2 calls something so that the NaN value is subjected to canonicalization.
![]() |
Reporter | |
Comment 3•8 years ago
|
||
It is precisely due to non-deterministic reasons that we need the --enable-more-deterministic flag. Can this be ignored somehow? Unless you are mentioning that we can only test wasm without this flag.
Comment 4•8 years ago
|
||
We had this idea in more deterministic mode to canonicalize at all the places where floating point values could be generated, to eliminate non-determinism entirely: f32/f64.const, conversion from * to floating-point, floating-point loads, etc. Probably I should do that now, since I've seen this fuzz bug too. Thanks for the report Gary! Glad awsm can integrate in regular fuzzers too.
Comment 5•8 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3) > It is precisely due to non-deterministic reasons that we need the > --enable-more-deterministic flag. Can this be ignored somehow? Unless you > are mentioning that we can only test wasm without this flag. Well, you can test Baldr with this flag but not (currently) Rabaldr, the baseline compiler. Once Benjamin is happy with whatever he's doing for Baldr maybe I can port it over, if he doesn't feel the call to do so himself :)
Comment 6•8 years ago
|
||
WIP patch that does canonicalization at the masm level, so everything is commoned out for rabaldr/badlr. Have to extensively test it. This covers: - FP constants - reinterprets (GPR => FP) - FP loads - FP globals I think there are the only sources of non-deterministic NaNs in wasm, assuming most FP CPU instructions which get a NaN as inputs return one of the NaN inputs.
![]() |
Reporter | |
Comment 7•7 years ago
|
||
Note to self: Bug 1277562 comment 59 removed --wasm-always-baseline and added --no-wasm-baseline instead. This is in the following changeset: https://hg.mozilla.org/mozilla-central/rev/9ea44ef0c07c
![]() |
Reporter | |
Comment 8•7 years ago
|
||
autoBisect shows this is probably related to the following changeset: The first good revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/0f4d52995594 user: Lars T Hansen date: Thu Feb 09 15:15:17 2017 +0100 summary: Bug 1277562 - Part 9: Add Wasm Tier 2 compilation tasks. r=luke Lars, is bug 1277562 a likely fix?
Flags: needinfo?(bbouvier) → needinfo?(lhansen)
Comment 9•7 years ago
|
||
The revision that appeared to fix in fact fixed nothing, it just masked the problem. It's now timing-dependent whether baseline code runs or ion code runs. To test baseline-only, run with --no-wasm-ion. To test ion-only, run --no-wasm-baseline. Otherwise you get tiering which means you usually don't know which code you're running. Fuzzers need to take this into account.
Flags: needinfo?(lhansen)
![]() |
Reporter | |
Comment 10•7 years ago
|
||
You're right, it still exists: $ ./js-dbg-64-dm-linux-d53ba311ca2f --fuzzing-safe --no-threads --ion-eager --no-wasm-ion 1346076.js 2139095041 $ ./js-dbg-64-dm-linux-d53ba311ca2f --fuzzing-safe --no-threads --ion-eager --no-wasm-baseline 1346076.js 2143289344
Updated•7 years ago
|
Flags: needinfo?(bbouvier)
Updated•6 years ago
|
Component: JavaScript Engine: JIT → Javascript: Web Assembly
Comment 11•6 years ago
|
||
After thinking about this for some (long) time, I think my fuzzer shouldn't generate test cases that show NaN-payload related differential behavior, because this is unlikely to be an error in general (the spec is a bit blurry about what's to happen after certain operators). So the fuzzer should be fixed, not Spidermonkey. The way to fix the fuzzer is to canonicalize NaNs after every single operation that could produce a floating point number (that's what binaryen fuzz does, as far as I know).
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bbouvier)
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•