1451976 - Differential Testing: Different output message involving Math.fround and 2 ** 53 - 2

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

setJitCompilerOption("ion.forceinlineCaches", 1);
function f(y, z) {
    return Math.fround(z) < ~y;
};
var x = [2 ** 53 - 2, 0];
for (var i = 0; i < 3; ++i) {
    print(f(x[0], x[1]));
}


$ ./js-dbg-32-dm-linux-7b40283bf1c7 --fuzzing-safe --no-threads --ion-eager testcase.js
true
true
false

$ ./js-dbg-32-dm-linux-7b40283bf1c7 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
true
true
true
$

Tested this on m-c rev 7b40283bf1c7.

My configure flags are:

CC="gcc -m32 -msse2 -mfpmath=sse" CXX="g++ -m32 -msse2 -mfpmath=sse" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh ./configure --target=i686-pc-linux --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u -m funfuzz.js.compile_shell -b "--enable-more-deterministic --enable-debug --32" -r 7b40283bf1c7

Still trying to get a bisection for now.

I found this deep in the results after the addition of support for fuzzing new boundary value "2 ** 53 - 2" to fix funfuzz issue #90:
https://github.com/MozillaSecurity/funfuzz/issues/90

Setting s-s as a start since this seems to involve boundary values, to be safe.

Setting needinfo? from Jan as a start while I try to get a bisection... (Something broke recently)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e7b45cdbc1a5
user:        Matthew Gaudet
date:        Wed Feb 07 14:22:48 2018 -0500
summary:     Bug 1434717: Part 6: Implement UnaryArith IC for doubles r=tcampbell

Matthew, is bug 1434717 a likely regressor?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This seems to block fuzzing with compare_jit.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

So, this test case passes without --enable-more-deterministic

When making the ABI call under enable-more-deterministic, the ABI emission code ends up calling via storeDouble into canonicalizeDoubleIfDeterministic. 

During this check, 2^53-2 is being flagged as a NaN, and replaced with GenericNaN. 

Now: It's not clear to me where the bug here is. Seemingly everyone is checking for NaNs with a variant of this: 

 branchDouble(DoubleOrdered, reg, reg, &notNaN);

As far as I understand, 2**53 -2 is 9007199254740990 which is not a double NaN (though, the first bit _is_ a float NaN). 

The emitted code is: 

0x45acf613 ? ucomiss %xmm0,%xmm0     | Compare
0x45acf616 ? jnp    0x45acf624       | Jump
0x45acf61c ? movss  0x45acf720,%xmm0 //This is where the arg is transformed
0x45acf624 ? movss  %xmm0,(%esp)

When entering that section, here's xmm0: 

$42 = {
  v4_float = {[0] = -nan(0x7ffffe), [1] = 191.999985, [2] = 0, [3] = 0}, 
  v2_double = {[0] = 9007199254740990, [1] = 0}, 
  v16_int8 = {[0] = -2, [1] = -1, [2] = -1, [3] = -1, [4] = -1, [5] = -1, [6] = 63, [7] = 67, [8] = 0, [9] = 0, [10] = 0, [11] = 0, [12] = 0, [13] = 0, [14] = 0, [15] = 0}, 
  v8_int16 = {[0] = -2, [1] = -1, [2] = -1, [3] = 17215, [4] = 0, [5] = 0, [6] = 0, [7] = 0}, 
  v4_int32 = {[0] = -2, [1] = 1128267775, [2] = 0, [3] = 0}, 
  v2_int64 = {[0] = 4845873199050653694, [1] = 0}, 
  uint128 = 4845873199050653694
}

Reading up on ucomisss [1], not sure why this is flagging a NaN.

Why we're not impacted on Baseline, I'm not sure. 

[1]: https://c9x.me/x86/html/file_module_x86_id_317.html

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Oh, ucomiss is single precision. This needs to be double precision I think.

Comment 5

[Ted Campbell [:tcampbell]]

The issue is that in this test case we have a xmm0 in live set as a Float (unrelated to this IC) as Ion allows. In [1] we push FloatReg0 before using it and try to indicate the register as non-volatile (to avoid redundant push). The problem is that we set this for XMM0:Double and XMM0:Single remains in the live set. This hits [2] which in turn calls canonicalizeFloatIfDeterministic. This clobbers the XMM0 register and we fail the test.

[1] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/jit/CacheIRCompiler.cpp#1890,1898
[2] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp#312

This is only an --enable-more-deterministic failure so I don't believe is S-S.


Experimenting with adding |save.takeUnchecked(FloatRegister(X86Encoding::xmm0, FloatRegisters::Single));| as well fixes the issue. (The SIMD case also needs to be fixed). We certainly need more asserts for this stuff.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Ted Campbell [:tcampbell] from comment #5)
> This is only an --enable-more-deterministic failure so I don't believe is
> S-S.

Thanks, opening up. Still a [fuzzblocker] though - some of our fuzzers need --enable-more-deterministic to operate.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

I've got a temporary fix (https://hg.mozilla.org/try/rev/327fe7f09c39b3727be89db2cc8f6a16665bfede) floating through try right now. That should unblock the fuzzers. (I'll attach the test cases to the patch once we've passed try, and bug 1451984 (which will be a dupe) is unrestricted. 

I think there's a more fundamental question here about how we can correctly interact with Ion's live register sets when it comes to Floats.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Please also nominate the fix (quick fix or full fix) to version 60, which is slated to be the next ESR.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Temporary fix for --enable-more-deterministic when doing DoubleNot IC

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8966412 [details] [diff] [review]
Temporary fix for --enable-more-deterministic when doing DoubleNot IC

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

Great. This should unblock fuzzing with --enable-more-deterministic. Builds without the flag are unaffected.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de33fb39fa48
Temporary fix for --enable-more-deterministic when doing DoubleNot IC r=tcampbell

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on cacheir/bug1451984.js:7

backout:

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=de33fb39fa4878714120d06ce6a0c72179016be9&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172845395&repo=mozilla-inbound&lineNumber=33760

[task 2018-04-10T12:01:19.503Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true
[task 2018-04-10T12:01:19.504Z] Stack:
[task 2018-04-10T12:01:19.504Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1
[task 2018-04-10T12:01:19.504Z] Exit code: 3
[task 2018-04-10T12:01:19.505Z] FAIL - cacheir/bug1451984.js
[task 2018-04-10T12:01:19.505Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/cacheir/bug1451984.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true (code 3, args "") [0.1 s]
[task 2018-04-10T12:01:19.506Z] {"action": "test_start", "pid": 3712, "source": "jittests", "test": "cacheir/bug1451984.js", "thread": "main", "time": 1523361679.3797631}
[task 2018-04-10T12:01:19.506Z] {"action": "test_end", "extra": {"jitflags": ""}, "message": "/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true", "pid": 3712, "source": "jittests", "status": "FAIL", "test": "cacheir/bug1451984.js", "thread": "main", "time": 1523361679.503204}
[task 2018-04-10T12:01:19.506Z] INFO exit-status     : 3
[task 2018-04-10T12:01:19.506Z] INFO timed-out       : False
[task 2018-04-10T12:01:19.506Z] INFO stdout          > 1
[task 2018-04-10T12:01:19.507Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true
[task 2018-04-10T12:01:19.507Z] INFO stderr         2> Stack:
[task 2018-04-10T12:01:19.507Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1
[task 2018-04-10T12:01:19.519Z] 1
[task 2018-04-10T12:01:19.519Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true
[task 2018-04-10T12:01:19.519Z] Stack:
[task 2018-04-10T12:01:19.519Z]   @/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1
[task 2018-04-10T12:01:19.519Z] Exit code: 3
[task 2018-04-10T12:01:19.519Z] FAIL - cacheir/bug1451984.js
[task 2018-04-10T12:01:19.519Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/cacheir/bug1451984.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true (code 3, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads") [0.1 s]
[task 2018-04-10T12:01:19.520Z] {"action": "test_start", "pid": 3712, "source": "jittests", "test": "cacheir/bug1451984.js", "thread": "main", "time": 1523361679.38885}
[task 2018-04-10T12:01:19.520Z] {"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads"}, "message": "/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true", "pid": 3712, "source": "jittests", "status": "FAIL", "test": "cacheir/bug1451984.js", "thread": "main", "time": 1523361679.519155}
[task 2018-04-10T12:01:19.520Z] INFO exit-status     : 3
[task 2018-04-10T12:01:19.520Z] INFO timed-out       : False
[task 2018-04-10T12:01:19.520Z] INFO stdout          > 1
[task 2018-04-10T12:01:19.520Z] INFO stderr         2> /builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1 Error: Assertion failed: got (void 0), expected true
[task 2018-04-10T12:01:19.520Z] INFO stderr         2> Stack:
[task 2018-04-10T12:01:19.520Z] INFO stderr         2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/cacheir/bug1451984.js:7:1

Comment 14

[Natalia Csoregi [:nataliaCs]]

(In reply to Natalia Csoregi [:nataliaCs] from comment #13)
> Backed out for failures on cacheir/bug1451984.js:7
> 
> backout:
> 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b777948153fc

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Temporary fix for --enable-more-deterministic when doing DoubleNot IC

Comment 16

[Matthew Gaudet (he/him) [:mgaudet]]

-headdesk-

Ted: Changed the test case to actually return the value rather than print it. I had verified the test case by hand before doing the conversion, but forgot to re-run before submitting the patch; and I landed it this morning on the basis of a clean try run that hadn't had these tests added yet. 

Here's the new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=224ef2beadc6c5f30d3b3db324ee47bfe574d6dc

(Apologies to everyone)

Comment 17

[Matthew Gaudet (he/him) [:mgaudet]]

There's a question here about what the long-term, correct fix is. 

The most simple version of the problem (that I think is correct) goes like this: 

We are attempting to attach a DoubleNot CacheIR IC in Ion, at a point where Ion has XMM0:Single as a live register. 

When we go to make a runtime call, we preserve registers using masm.PushRegsInMask. The mask produced of course contains XMM0:Single as a live register, so save-and restore code is generated. 

The problem is that we are using XMM0:Double at this moment as our argument. It was preserved earlier with an explicit push [1], and contains a value that is a valid double, but when treated as a Float is a NaN. To avoid double-preservation, the code does a takeUnchecked of FloatReg0 (XMM0:Double) [2]. 

However, XMM0:Single is still preserved because it is live in the Ion code. The XMM0:Single preservation code hits canonicalizeFloatIfDeterministic [3], which maps all float NaNs to GenericNaN. The problem is, this ends up clobbering XMM0:Double. 

The temporary fix in Comment 15 does a takeUnchecked of XMM0:Single, thereby preventing preservation, and the generation of the clobbering code under enable-more-deterministic. This feels like a hack though, and an incomplete one at that, as there's an equivalent concern if XMM0:Simd128 is live. 

Any thoughts Nicolas? 


[1]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRCompiler.cpp#1889
[2]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRCompiler.cpp#1898
[3]: https://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler-inl.h#719

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 8966559 [details] [diff] [review]
Temporary fix for --enable-more-deterministic when doing DoubleNot IC

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

Oops =\

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc5b713ca78
Temporary fix for --enable-more-deterministic when doing DoubleNot IC. r=tcampbell

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/afc5b713ca78

Comment 21

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> Please also nominate the fix (quick fix or full fix) to version 60, which is
> slated to be the next ESR.

Despite the date on the commit, I think this actually landed after the 60 split (it was backed out of 60 I believe). I will verify it's not in 60 tho.

Comment 22

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, https://hg.mozilla.org/mozilla-central/rev/e7b45cdbc1a5 says first release with is 61, so I don't think this will need uplift.

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #17)
> The temporary fix in Comment 15 does a takeUnchecked of XMM0:Single, thereby
> preventing preservation, and the generation of the clobbering code under
> enable-more-deterministic. This feels like a hack though, and an incomplete
> one at that, as there's an equivalent concern if XMM0:Simd128 is live. 

One idea is to do what we discussed in another bug: make sure the LIR instruction has a double temp register (either a fixed one or an arbitrary one), then assert this in the CacheIR code and use it without saving/restoring it.

Comment 24

[Matthew Gaudet (he/him) [:mgaudet]]

That's a nice clean solution (not sure how I'd write the CacheIR code to handle arbitrary float temps, but certainly could preferentially allocate FloatReg0). 

My immediate gut concern is that it could have non-local performance impact on Ion code, as it will force al_ UnaryArith ICs to take a FloatRegister, even though it may not be used. Not sure if that's actually a real concern. 

Will ponder.

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #22)
> Ok, https://hg.mozilla.org/mozilla-central/rev/e7b45cdbc1a5 says first
> release with is 61, so I don't think this will need uplift.

Thanks for verifying this!

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #17)
> …
> 
> Any thoughts Nicolas? 

Discussing the problem with Matthew yesterday, …

The aligned&dominated mask of FloatReg0:double should cover the xmm0:single, xmm0:double and xmm0:quad variants, as well as the s0, s1, d0 and q0 (not yet implemented) variants.

What can be done is spilling any live-reg which alias (and-mask) the aligned&dominated mask of FloatReg0:double, and removing it from the live-set to be spilled later on.