Closed
Bug 1073910
Opened 10 years ago
Closed 9 years ago
Differential Testing: Different output message involving Math.round, Math.fround and Math.imul
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: gkw, Assigned: bbouvier)
References
Details
(Keywords: regression, testcase)
Attachments
(1 file, 1 obsolete file)
6.09 KB,
patch
|
mjrosenb
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
function f(y) { print(Math.round(Math.fround(Math.imul(-0xE, y)))) } f(0xf) f(0xfffff) $ ./js-dbg-opt-32-dm-nsprBuild-sfp-linux-6a63bcb6e0d3 --fuzzing-safe --no-threads --ion-eager testcase.js -210 -14680049 $ ./js-dbg-opt-32-dm-nsprBuild-sfp-linux-6a63bcb6e0d3 --fuzzing-safe --no-threads --baseline-eager testcase.js -210 -14680050 Tested this on m-c rev 6a63bcb6e0d3. (on 64-bit Mac, the second line both reads -14680050) My configure flags are: CC="gcc-4.7 -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7" CXX="g++-4.7 -mfloat-abi=softfp -B/usr/lib/gcc/arm-linux-gnueabi/4.7" AR=ar sh /home/fuzz5lin/trees/mozilla-central/js/src/configure --target=arm-linux-gnueabi --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests Marty/Douglas, any idea what's going on here? This seems to go back beyond Jul 2014 (http://hg.mozilla.org/mozilla-central/rev/e8558ecd9b16).
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
Reporter | ||
Comment 1•10 years ago
|
||
Actually it goes back beyond Apr 2014, so it's likely to also affect Fx31: https://hg.mozilla.org/mozilla-central/rev/5ad5f92387a2
Reporter | ||
Updated•10 years ago
|
status-firefox35:
--- → affected
Comment 2•10 years ago
|
||
TLDR: using '+' in fp rounding is almost guaranteed to be wrong.
Looks like the 'straightforward' translation from double to float32 is at fault here. I probably should have caught this during review considering I've found nearly identical bugs two other places in our fp rounding code!
For reference, here is the generated code for fround(x:s0):
> 0xf7262828 vcmp.f32 s0, #0 (bool b = x < 0)
> 0xf726282c vmov.f32 s30, #5.000000e-01
> 0xf7262830 vabs.f32 s1, s0
> 0xf7262834 vadd.f32 s1, s30, s1 (calculate fabs(x) + 0.5)
> 0xf7262838 vmrs APSR_nzcv, fpscr
> 0xf726283c beq #24
> 0xf7262840 bmi #36 (we actually take this branch)
> 0xf7262844 bvs #832
> 0xf7262848 vcvt.u32.f32 s30, s1
> 0xf726284c vmov r2, s30
> 0xf7262850 movs r2, r2
> 0xf7262854 bmi #816
> 0xf7262858 b #44
> 0xf726285c vmov r2, s0
> 0xf7262860 cmp r2, #0
> 0xf7262864 bne #800
> 0xf7262868 b #28
> 0xf726286c vcvt.u32.f32 s30, s1 (branch jumps here, round fabs(x)+0.5 to an integer)
> 0xf7262870 vmov r2, s30 (declare this value 'close to correct', well, its negation)
> 0xf7262874 vcvt.f32.u32 s30, s30 (convert this value back to a float)
> 0xf7262878 vcmp.f32 s30, s1 (check if fabs(x) + 0.5 == float(int(fabs(x) + 0.5)))
> 0xf717d87c vmrs APSR_nzcv, fpscr (arm is kind of strange, you need to copy the fp condition codes into the integer codes)
> 0xf717d880 subeq r2, r2, #1 (if the two values are equal, we were exactly 0.5 away from an integer, and should round towards 0, so subtract 1)
> 0xf717d884 rsbs r2, r2, #0 (negate the value)
now, the issue is the value that we have, x=14680050 is large enough that 14680050+0.5 == 14680050, so the 'exactly N+0.5' incorrectly says that we have a quantity that needs to be fixed. While the double conversion theoretically has a case like this, it occurs around 2^52, well outside of the range of int32.
Fixing this will probably not be fun, unless we can just convert the fp value to a double, then perform double rounding. It'll probably take longer, singe double math is slower than float32 math in general, but as long as it is spec compliant, it is the fastest way I see towards a solution.
Flags: needinfo?(mrosenberg) → needinfo?(benj)
Comment 3•10 years ago
|
||
Oh, and the bug has existed since we started jitting roundf on arm: changeset: 171239:a1b396e1f1dd user: Benjamin Bouvier <benj@benj.me> date: Fri Feb 28 12:07:05 2014 +0100 summary: Bug 930477: Specialize Round for Float32; r=jandem,mjrosenb
Reporter | ||
Comment 4•10 years ago
|
||
The fuzzers only found this now because bug 1000606 was fixed only early this month.
Assignee | ||
Comment 5•10 years ago
|
||
I can do it when i'll have some spare cycles, which isn't the case right now. Anybody can feel free to take in the meanwhile.
Flags: needinfo?(benj)
Reporter | ||
Comment 6•10 years ago
|
||
Jan, who might be a good person to take this? FWIW this blocks finding differential testing bugs involving Math.round, Math.fround and Math.imul.
Flags: needinfo?(jdemooij)
Comment 7•10 years ago
|
||
Benjamin said he'd take a look soon.
Flags: needinfo?(jdemooij) → needinfo?(benj)
Assignee | ||
Comment 8•9 years ago
|
||
So this implements the workaround you've mentioned on irc yesterday (don't do the adjustment if x + .5 == x), and it seems to work just fine. I was wondering why the almighty IEEE754 Gods would spare us on x86 and x64. On these platforms, we add .5 and *then* use CVTTSS2SI, which (as its name indicates), truncates inputs that can't be exactly represented as an int32 towards zero. Then we test if the original input was an integer and adjust the result by one if necessary. Adding 0.5 or not doesn't change the result of CVTSS2SI + adjustment (especially if the input plus 0.5 == the input), so we seem to be fine there. The tests suite on ARM locally passes (normal mode and --ion). I've made another script that has tested all integers (evaluated as float32, i.e. Math.fround()'d) between 0 and ~-40 millions, and it worked fine. I'll try-run this patch once try re-opens, just in case the almighty Gods of IEEE754 don't change their minds in the meanwhile.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(dtc-moz)
Flags: needinfo?(benj)
Attachment #8529878 -
Flags: review?(mrosenberg)
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2805e643c803
Comment 10•9 years ago
|
||
Comment on attachment 8529878 [details] [diff] [review] Patch + tests Review of attachment 8529878 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/MacroAssembler-arm.cpp @@ +4633,1 @@ > I don't see a check for the X + 0.499999 == x case here, I'm somewhat certain that it can come up. If I'm wrong, feel free to re-r? for a quick review. @@ +4648,5 @@ > > bind(&handleNeg); > > // Add 0.5 to negative numbers, storing the result into tmp. > + ma_vabs_f32(input, tmp); Since this is explicitly the negative case, I'd use vneg rather than vabs.
Attachment #8529878 -
Flags: review?(mrosenberg) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Good remark. Actually I think it doesn't matter. The initial issue for the *negative case* is that we had to prevent from doing the fixup (sub 1) in the case where x + .5 === x. We don't have such a fixup in the *positive case*, so there's nothing to test. Added a comment there, but not sure about the wording -- it's always a weird thing to say "hey, look, we don't need to have this piece of code here that is present there below for the other case". Of course, if you can think of any case that will show an issue with the positive case, I'd be happy to fix my patch.
Attachment #8529878 -
Attachment is obsolete: true
Attachment #8534497 -
Flags: review?(mrosenberg)
Updated•9 years ago
|
Attachment #8534497 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05bc6213e41
https://hg.mozilla.org/mozilla-central/rev/c05bc6213e41
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 14•9 years ago
|
||
I'll request uplift in a few days, unless fuzzers find something else about this code path in the meanwhile.
Flags: needinfo?(benj)
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8534497 [details] [diff] [review] Patch + tests Approval Request Comment [Feature/regressing bug #]: Bug 930477 [User impact if declined]: small correctness issues on ARM in C++ programs compiled to JS with emscripten (and all other programs using Math.fround/Math.round together, on ARM) [Describe test coverage new/current, TBPL]: test case in tree, all tests passed on TBPL and locally, patch sticked for 3 days as of today [Risks and why]: very low, if not none [String/UUID change made/needed]: n/a
Flags: needinfo?(benj)
Attachment #8534497 -
Flags: approval-mozilla-release?
Attachment #8534497 -
Flags: approval-mozilla-beta?
Attachment #8534497 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8534497 -
Flags: approval-mozilla-beta?
Attachment #8534497 -
Flags: approval-mozilla-beta+
Attachment #8534497 -
Flags: approval-mozilla-aurora?
Attachment #8534497 -
Flags: approval-mozilla-aurora+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1108b828c512 https://hg.mozilla.org/releases/mozilla-beta/rev/1063fd042031 Should we consider taking this on b2g34 for v2.1? Especially since it's ARM?
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Flags: in-testsuite+
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #16) > Should we consider taking this on b2g34 for v2.1? Especially since it's ARM? Yes. If we need another approval for b2g34, the approval comment is in bug 15.
Comment 18•9 years ago
|
||
Comment on attachment 8534497 [details] [diff] [review] Patch + tests if this is already fixed on 35, we don't need this for mozilla-release.
Attachment #8534497 -
Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in
before you can comment on or make changes to this bug.
Description
•