Closed Bug 458431 Opened 12 years ago Closed 12 years ago

Expression-ordering bug in js_Math_max triggered by 0,-0

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: graydon, Assigned: graydon)

Details

Attachments

(2 files)

A subtle expression-order bug in the copy of the Math.max code specialized to doubles in jsbuiltins.cpp. Triggered very indirectly depending on the state of your FPU on trace, and possibly the phase of the moon.

Proposed fix includes comments to both copies to clarify the rationale.
Attachment #341655 - Flags: review?(mrbkap)
Comment on attachment 341655 [details] [diff] [review]
fix to jsbuiltins.cpp and comment to it as well as older copy in jsmath.cpp.

>diff -r 4aa52b9b0ea3 js/src/jsbuiltins.cpp
>+    /* Note: it is essential that you write the ternary expression here such that the false

Nit: start both comments as:
/*
 * Note: ...

r=mrbkap with that.
Attachment #341655 - Flags: review?(mrbkap) → review+
Pushed to tracemonkey as revision 10e607498653.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I'm reopening this for two reasons. First, jorendorff's recent landing of bug 458735 caused this to regress, and second, now that I stare at it I'm not really sure I know what to expect the generated code to "always" do in the first situation. Disassembling the object file, we get a FUCOMPP, which intel's docs explicitly say "treat 0 and -0 as equal". Meaning all ordering compares will be false, and my original comment will be correct. But then: how does it *ever* work the first N passes through the loop, or on other tester's computers? Different compiler choices? False documentation? Mysterious.

My conservative (and plainly ill-informed) choice was to re-code the problem such that there is no reliance on *any* behavior of 0 > -0, in either order, be it always true, always false, or some combination thereof. New patch uses copysign exclusively for any case -- min or max -- involving 0 and -0 simultaneously.

Objections? Comments?
Assignee: general → graydon
Status: RESOLVED → REOPENED
Attachment #342356 - Flags: review?(mrbkap)
Resolution: FIXED → ---
Attachment #342356 - Flags: review?(mrbkap) → review+
Comment on attachment 342356 [details] [diff] [review]
possibly different correction for same issue

>+    if (p == 0 && p == d) {
>+        if (fd_copysign(1.0, d) == -1)
>+            return p;
>+        else
>+            return d;

Nit: avoid else-after-if.

r=mrbkap with that.
Er, else-after-return obviously :-/
The approved fix here went in accidentally with bug 457786, revision 53072c29a4fe. Subsequently pushed a lingering typo-fix in revision 1df09570a2fa.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.