995542 - Incorrect result in OdinMonkey on fuzz testcase

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Attachment: emcc-7-registerize.js

Attached is a testcase from Moh's fuzzer. It gives wrong results in SpiderMonkey, ion seems the culprit (disabling odin does nothing, disabling ion causes correct results).

Note in the testcase the line

  //hashMemory(1);

If that is uncommented, then the results are correct, so that must affect optimization in some way that avoids the bug.

Comment 1

[Jan de Mooij [:jandem]]

Alon, this is what I'm seeing with inbound rev 7ac91d7d6269 on 32-bit OS X:

d8: 3008991155
js --no-asmjs --no-baseline --no-ion: 3008991155

So this seems to be the correct result.

js --no-asmjs: 3008991155
js --no-baseline --no-ion: 3187808385

So it works fine with asm.js disabled, but it fails with asm.js enabled even if Baseline/Ion are disabled. Right?

Comment 2

[Jan de Mooij [:jandem]]

Oh it fails with both LSRA and backtracking. It fails with LICM or range analysis disabled. Disabling GVN seems to fix it (--no-baseline --no-ion --ion-gvn=off):
 
res = 3008991155

Comment 3

[Alon Zakai (:azakai)]

Yes, I get those results too.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Alon Zakai (:azakai) from comment #3)
> Yes, I get those results too.

Thanks, I asked in case we were seeing different bugs. Updating summary.

Comment 5

[Jan de Mooij [:jandem]]

Gary maybe you could use autoBisect and/or lithium on this if you have some spare cycles? The attached testcase prints:

--no-asmjs: 3008991155 (good)
(no flags): 3187808385 (bad)

Comment 6

[Luke Wagner [:luke]]

I'm currently narrowing where this goes wrong in the test case, hopefully not too much longer.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm currently using Lithium to reduce this.

Comment 8

[Luke Wagner [:luke]]

Attachment: emcc-7-registerize.js (offending Math.imul)

I tracked this down to a specific Math.imul that is getting eliminated (I'm not sure with what; all the other imuls in the loop seem to have different inputs).  In the attached testcase, search for calls to asmPrintInt().  The print shows the inputs are the same w/ and w/o --ion-gvn, but the output is different.  Stepping through the generated code, the Math.imul is getting replaced by a load from the stack that has the wrong value.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 600-line testcase

$ ./js-opt-64-ts-darwin-265d82091bce --fuzzing-safe --ion-parallel-compile=off emcc-7-registerize.js
res = 31878083853187808385u

$ ./js-opt-64-ts-darwin-265d82091bce --fuzzing-safe --ion-parallel-compile=off --no-asmjs emcc-7-registerize.js
res = 30089911553008991155u

Comment 10

[Luke Wagner [:luke]]

Looking at the code some more, it looks like both Math_imul(i5,i5) and Math_imul(i3,i3) are being incorrectly eliminated (I assume by GVN) and the results of Math_imul(i17,i17) are being used.  Despite the whole file being quite large due to glue code, the actual offending loop with the imuls is pretty short and simple; Marty perhaps you could take a look at this?

Comment 11

[Moh Haghighat]

Attachment: The C version of the test from which emcc has generated the JS test

Comment 12

[Moh Haghighat]

Attachment: Input for the C version of the test

Comment 13

[Marty Rosenberg [:mjrosenb]]

looking at the logs, this is almost certainly the cause of the issue:
[GVN] Broke congruence for instruction 333 (0x1d855d0) with VN 335 (now using 333)
[GVN] marked 398
[GVN] marked 358
[GVN] marked 352
[GVN] unmarked 335
[GVN] Broke congruence for instruction 335 (0x1d86170) with VN 335 (now using 333)
Looking into the code that caused this.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/fab7a1635246
user:        Marty Rosenberg
date:        Wed May 01 10:01:55 2013 -0700
summary:     Bug 863867 - Don't assume that two MDefs with the same value number are in the same congruence class (r=sstangl)

Marty, is bug 863867 a likely regressor?

Comment 15

[Marty Rosenberg [:mjrosenb]]

Attachment: AddMoreConsumerstoGVN-r0.patch

Comment 16

[Marty Rosenberg [:mjrosenb]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> autoBisect shows this is probably related to the following changeset:
> 
> The first bad revision is:
> changeset:   http://hg.mozilla.org/mozilla-central/rev/fab7a1635246
> user:        Marty Rosenberg
> date:        Wed May 01 10:01:55 2013 -0700
> summary:     Bug 863867 - Don't assume that two MDefs with the same value
> number are in the same congruence class (r=sstangl)
> 
> Marty, is bug 863867 a likely regressor?

Strange.  I didn't think submitting a patch would clear needinfo.  Anyhow, I find it highly unlikely that we regressed from that particular patch.  I'd say this bug was here before that, and the patch somehow or other exposed it.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Since it was failing also in 28 and it is too late in the 29 cycle, we won't accept the patch for 29.
ESR only gets security fixes (and I don't think this one is).
However, we are tracking it for the next releases since asm.js is becoming more and more important.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 60-line testcase involving --ion-eager

Marty, perhaps you'd like to see if you can further tune this almost-fully-reduced testcase to land as a test?

$ ./js-opt-64-dm-ts-darwin-265d82091bce --fuzzing-safe --ion-parallel-compile=off emcc-7-registerize.js
[object Object]

$ ./js-opt-64-dm-ts-darwin-265d82091bce --fuzzing-safe --ion-parallel-compile=off --ion-eager emcc-7-registerize.js

$

Tested on Mac:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

Comment 19

[Sean Stangl [:sstangl]]

Comment on attachment 8406551 [details] [diff] [review]
AddMoreConsumerstoGVN-r0.patch

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

Marty explained on IRC what this patch does, saving here for posterity:

15:06 < mjrosenb> yeah, so what happens there is we have three instructions, 
                  A1, A2, and A3, which are considered 'equivalent', and there 
                  are three instructions that use them, B1, B2, B3, where B1 
                  uses A1, B2 A2, and B3 A3.
15:06 < mjrosenb> A1 is the canonical member of its group, and B2 is the 
                  canonical member of its group.
15:07 < mjrosenb> then GVN finally decides that A1 is different from the rest 
                  of the group, moves it to its own group, and re-numbers A2 
                  and A3.
15:08 < mjrosenb> and all of the uses of A2 and A3 are added to the worklist.
15:08 < mjrosenb> namely, B2 and B3
15:09 < mjrosenb> both which are still equivalent to one another, and so don't 
                  get reprocessed.
15:09 < mjrosenb> and so B1 remains in the equivalence class with B2 and B3.
15:09 < mjrosenb> which is wrong.

Comment 20

[Marty Rosenberg [:mjrosenb]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea518e77780

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ea518e77780

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a Beta uplift nomination here to make this week's Beta?

Comment 23

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8406551 [details] [diff] [review]
AddMoreConsumerstoGVN-r0.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Adding gvn
User impact if declined: 
Strange testcases can produce incorrect results
Testing completed (on m-c, etc.): 
It's been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): 
some test cases can take a bit longer to run. I expect the differences to be unmeasurable.
String or IDL/UUID changes made by this patch:

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e2975cd6792

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9e2975cd6792