686323 - ion: div bug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: testcase

Ion seems to have some bug regarding, division by zero behavior.

Comment 1

[Tom S [:evilpie]]

Attachment: small fix for two issues

This should fix the constant folding (also it seems that this doesn't work?!).
Also i found one small optimization that we used to have in Jäger when checking for 0 and INT_MIN.

Looking at the spew it looks like the b/0 is optimized away?
Also i am not convinced that the inline code for div is valid (How did Jäger handle that?)

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 559869 [details] [diff] [review]
small fix for two issues

Review of attachment 559869 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/MIR.cpp
@@ +127,5 @@
> +            else
> +                ret.setNumber(js_PositiveInfinity);
> +        } else {
> +            ret.setNumber(d1 / d2);
> +        }

I see this is in JM too - could we factor it out into jsnum, like js_fmod?

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -476,1 +467,1 @@
> >      Label nonzero;

This removes the -2147483648 / -1 check:

$ ./Debug32/js --ion --ion-eager jit-test/tests/ion/testDivide.js
Floating point exception

Comment 3

[Tom S [:evilpie]]

>I see this is in JM too - could we factor it out into jsnum, like js_fmod?
Sure.

>This removes the -2147483648 / -1 check:
fail! i somehow read masm.cmpl(>rhs<, Imm32(INT_MIN)); all the freaking time.

Any idea what causes the other issues?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Note: jsinterp.cpp has the same logic too, so we could remove it from three places)

Are there issues other than the constant folding? testD() seems to pass for me.

Comment 5

[Tom S [:evilpie]]

>tom@tom-linux:~/ionmonkey/js/src/build-debug/shell$ ./js --ion test.js
>test.js:28: Error: Assertion failed: got 1, expected -Infinity

Comment 6

[Tom S [:evilpie]]

Attachment: patch for inbound

Comment 7

[Tom S [:evilpie]]

Attachment: ion

Comment 8

[Tom S [:evilpie]]

Do not resolve please, this bug isn't fixed yet.

The unrelated base patch is http://hg.mozilla.org/integration/mozilla-inbound/rev/c614dabf94e2.

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c614dabf94e2

(I'll nom this bug for the most obscure summary ever! :P)

Comment 10

[pete]

This code doesn't make any sense to me:


        if (a == 0 || JSDOUBLE_IS_NaN(a) 
#ifdef XP_WIN
            || JSDOUBLE_IS_NaN(a) /* XXX MSVC miscompiles such that (NaN == 0) */
#endif


I think the second JSDOUBLE_IS_NAN(a) should be JSDOUBLE_IS_NAN(b).

Also, it would surprise me if MSVC > 6 still botches NaNs like this.

Comment 11

[Sean Stangl [:sstangl]]

Comment 10 is correct. This needs fixing on inbound before m-c promotes to Aurora.

@dvander: OK to push the above change?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

pete, good catch, thanks!

sstangl, yeah please, r=me on that fix

Comment 13

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/362691b74d6c

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/362691b74d6c