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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

Attachment: fix to jsbuiltins.cpp and comment to it as well as older copy in jsmath.cpp.

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.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 2

[Graydon Hoare :graydon]

Pushed to tracemonkey as revision 10e607498653.

Comment 3

[Graydon Hoare :graydon]

Attachment: possibly different correction for same issue

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?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Er, else-after-return obviously :-/

Comment 6

[Graydon Hoare :graydon]

The approved fix here went in accidentally with bug 457786, revision 53072c29a4fe. Subsequently pushed a lingering typo-fix in revision 1df09570a2fa.