Closed Bug 1451976 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: gkw, Assigned: mgaudet)

References

Details

(Keywords: regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file, 1 obsolete file)

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)
Flags: needinfo?(jdemooij)
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?
Blocks: 1434717
Flags: needinfo?(jdemooij) → needinfo?(mgaudet)
This seems to block fuzzing with compare_jit.
Whiteboard: [fuzzblocker]
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
Oh, ucomiss is single precision. This needs to be double precision I think.
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.
(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.
Group: javascript-core-security
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.
Flags: needinfo?(mgaudet)
Duplicate of this bug: 1451984
Please also nominate the fix (quick fix or full fix) to version 60, which is slated to be the next ESR.
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
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.
Attachment #8966412 - Flags: review?(tcampbell) → review+
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de33fb39fa48
Temporary fix for --enable-more-deterministic when doing DoubleNot IC r=tcampbell
Keywords: checkin-needed
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
Flags: needinfo?(mgaudet)
(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
Attachment #8966412 - Attachment is obsolete: true
-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)
Flags: needinfo?(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
Flags: needinfo?(nicolas.b.pierron)
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 =\
Attachment #8966559 - Flags: review?(tcampbell) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc5b713ca78
Temporary fix for --enable-more-deterministic when doing DoubleNot IC. r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/afc5b713ca78
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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.
Ok, https://hg.mozilla.org/mozilla-central/rev/e7b45cdbc1a5 says first release with is 61, so I don't think this will need uplift.
(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.
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.
(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!
(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.
Flags: needinfo?(nicolas.b.pierron)
Depends on: 1466534
No longer depends on: 1466534
You need to log in before you can comment on or make changes to this bug.