Closed Bug 503595 Opened 11 years ago Closed 11 years ago

-0 w/ modulus disagreement on windows versus linux

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(1 file, 3 obsolete files)

In js shell:

x = -2 % -1
x %= -1
1/x

On Linux, this produces -Infinity. On Windows, it produces Infinity.

This is causing the current test breakage on Windows, in trace-tests (testModuloWithNegative1).
So, ECMA 262 section 5.2 says:

Mathematical operations such as addition, subtraction, negation, multiplication, division, and the mathematical functions defined later in this section should always be understood as computing exact mathematical results on mathematical real numbers, which do not include infinities and do not include a negative zero that is distinguished from positive zero.

But that's referring to arithmetic done directly in the spec.  That is, when the spec says +, it means real math plus, not 32-bit plus or IEEE plus or other such garbage.  "We'll write out the computery crap explicitly where appropriate."

So it's section 11.5.3 that has the rules that count:

• If either operand is NaN, the result is NaN.
• The sign of the result equals the sign of the dividend.
• If the dividend is an infinity, or the divisor is a zero, or both, the result is NaN.
• If the dividend is finite and the divisor is an infinity, the result equals the dividend.
• If the dividend is a zero and the divisor is finite, the result is the same as the dividend.

Thus, the first line computes a -0.  (Note that you need to say "uneval(x)" to see the -0.)

The second line also computes a -0.

The last line should produce a NaN, it seems to me.
Actually, I'm wrong.  The result of the expression "1/x" is section 11.5.2, not 11.5.3.  Those say:

"The sign of the result is positive if both operands have the same sign, negative if the operands have different signs."

and then:

"Division of a non-zero finite value by a zero results in a signed infinity. The sign is determined by the rule already stated above."

Thus, the correct result is -Infinity.

But please check my work.
I agree the correct result is -Infinity.

  // sign: negative, r = -2 - (2 * -1) = 0 ==> x = -0
  x = -2 % -1;

  // x = -0 % -1 ==> dividend 0, divisor finite ==> x = dividend = -0
  x = x % -1;

  // sign: negative, different signs, infinity/nonzero finite produces Infinity,
  // sign as previously determined (i.e. standard IEEE 754 rules)
  1 / x;
Attached patch possible fix (obsolete) — Splinter Review
Thanks to jimb's comments I'm fairly convinced that -0 % -1 should be producing -0. IE8 is doing this, despite the fact that Windows fmod(-0, -1) returns 0. Fx3 and 3.5 are producing 0 on Windows, and -0 on Linux/OS X.

Attached is a patch that adds another special Windows case to fmod(). It'd probably be nicer if callers of fmod() just used js_dmod though.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #387983 - Flags: review?(brendan)
Attached patch static inline (obsolete) — Splinter Review
Attachment #387983 - Attachment is obsolete: true
Attachment #388088 - Flags: review?(sayrer)
Attachment #387983 - Flags: review?(brendan)
Attachment #388088 - Attachment is obsolete: true
Attachment #388089 - Flags: review?(sayrer)
Attachment #388088 - Flags: review?(sayrer)
Attached patch irc nitsSplinter Review
Attachment #388089 - Attachment is obsolete: true
Attachment #388090 - Flags: review?(sayrer)
Attachment #388089 - Flags: review?(sayrer)
Comment on attachment 388090 [details] [diff] [review]
irc nits

I'll r+ this for now to fix tinderbox bustage. needs brendan or other peer to take a look, though.
Attachment #388090 - Flags: review?(sayrer)
Attachment #388090 - Flags: review?(brendan)
Attachment #388090 - Flags: review+
Comment on attachment 388090 [details] [diff] [review]
irc nits

Looks ok to me.
Attachment #388090 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/e502974c740b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 388090 [details] [diff] [review]
irc nits

Don't need a me-too from me here ;-).

/be
Attachment #388090 - Flags: review?(brendan) → review+
You need to log in before you can comment on or make changes to this bug.