Closed Bug 1073910 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving Math.round, Math.fround and Math.imul

Categories

(Core :: JavaScript Engine: JIT, defect, major)

ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- affected
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
firefox-esr31 --- affected
b2g-v1.4 --- unaffected
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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)
Actually it goes back beyond Apr 2014, so it's likely to also affect Fx31:

https://hg.mozilla.org/mozilla-central/rev/5ad5f92387a2
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)
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
The fuzzers only found this now because bug 1000606 was fixed only early this month.
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)
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)
Benjamin said he'd take a look soon.
Flags: needinfo?(jdemooij) → needinfo?(benj)
Attached patch Patch + tests (obsolete) — Splinter Review
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)
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-
Attached patch Patch + testsSplinter Review
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)
Attachment #8534497 - Flags: review?(mrosenberg) → review+
https://hg.mozilla.org/mozilla-central/rev/c05bc6213e41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
I'll request uplift in a few days, unless fuzzers find something else about this code path in the meanwhile.
Flags: needinfo?(benj)
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?
Attachment #8534497 - Flags: approval-mozilla-beta?
Attachment #8534497 - Flags: approval-mozilla-beta+
Attachment #8534497 - Flags: approval-mozilla-aurora?
Attachment #8534497 - Flags: approval-mozilla-aurora+
(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 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.