Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 8 years ago

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?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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.

	Lars T Hansen [:lth]
	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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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.

	Benjamin Bouvier [:bbouvier] (inactive)
	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.

	Lars T Hansen [:lth]
	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 :)

	Benjamin Bouvier [:bbouvier] (inactive)
	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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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?

	Lars T Hansen [:lth]
	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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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

	Benjamin Bouvier [:bbouvier] (inactive)
	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).