Closed Bug 686323 Opened 13 years ago Closed 13 years ago

ion: div bug

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: evilpie, Assigned: evilpie)

Details

Attachments

(3 files, 1 obsolete file)

Attached file testcase
Ion seems to have some bug regarding, division by zero behavior.
Attached patch small fix for two issues (obsolete) — Splinter Review
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 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
>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?
(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.
>tom@tom-linux:~/ionmonkey/js/src/build-debug/shell$ ./js --ion test.js
>test.js:28: Error: Assertion failed: got 1, expected -Infinity
Attachment #559949 - Flags: review?(dvander)
Attached patch ionSplinter Review
Attachment #559869 - Attachment is obsolete: true
Attachment #559949 - Flags: review?(dvander) → review+
Assignee: general → evilpies
Do not resolve please, this bug isn't fixed yet.

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

(I'll nom this bug for the most obscure summary ever! :P)
Target Milestone: --- → mozilla9
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 10 is correct. This needs fixing on inbound before m-c promotes to Aurora.

@dvander: OK to push the above change?
pete, good catch, thanks!

sstangl, yeah please, r=me on that fix
https://hg.mozilla.org/mozilla-central/rev/362691b74d6c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.