Closed Bug 995542 Opened 5 years ago Closed 5 years ago

Incorrect result in OdinMonkey on fuzz testcase

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 - wontfix
firefox30 + fixed
firefox31 + fixed
firefox-esr24 - wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: azakai, Assigned: mjrosenb)

References

Details

Attachments

(7 files)

Attached file 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.
Flags: needinfo?(jdemooij)
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?
Flags: needinfo?(jdemooij) → needinfo?(azakai)
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
Flags: needinfo?(luke)
Yes, I get those results too.
Flags: needinfo?(azakai)
(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.
Summary: Incorrect result in IonMonkey on fuzz testcase → Incorrect result in OdinMonkey on fuzz testcase
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)
Flags: needinfo?(gary)
I'm currently narrowing where this goes wrong in the test case, hopefully not too much longer.
Flags: needinfo?(luke)
I'm currently using Lithium to reduce this.
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.
Attached file 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
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?
Flags: needinfo?(mrosenberg)
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.
Flags: needinfo?(mrosenberg)
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?
Blocks: 863867
Flags: needinfo?(mrosenberg)
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Attachment #8406551 - Flags: review?(sstangl)
Flags: needinfo?(mrosenberg)
(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.
No longer blocks: 863867
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.
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>
Flags: needinfo?(gary) → needinfo?(mrosenberg)
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.
Attachment #8406551 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/5ea518e77780
Assignee: nobody → mrosenberg
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Can we get a Beta uplift nomination here to make this week's Beta?
Flags: needinfo?(mrosenberg)
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:
Attachment #8406551 - Flags: approval-mozilla-beta?
Flags: needinfo?(mrosenberg)
Attachment #8406551 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1011823
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.