983709 - Wrong results if JIT is enabled

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakub Galgonek]

Attachment: rmsd.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20140216133306

Steps to reproduce:

In my project, I want to compute RMSD by JavaScript. I have observed that my RMSD method returns a wrong result after several cycles. I have these troubles with firefox-24.3.0esr, if javascript.options.baselinejit.content is enabled. On the other hand, I did not have these troubles with firefox-27.0.1.

The HTML file with the JavaScript code is attached. The code has been generated by GWT originally, I have tried making it shorter.


Actual results:

The method is called in a for-loop for the same input data, and wrong results are obtained in some iterations. An error result is reported by a alert message ("error for i = ").


Expected results:

I expect that the results of the method will be the same in each iteration. Only a message "done" should by showed.

Comment 1

[Jan de Mooij [:jandem]]

We should bisect this probably.

Comment 2

[Alice0775 White]

Progression window(m-i)
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/057cd362da69
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909111349
Fixed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a1bd3bb5a0ba
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909111650
Progression Pushlog:(
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=057cd362da69&tochange=a1bd3bb5a0ba

Fixed by: 
a1bd3bb5a0ba	Hannes Verschore — Bug 909717: IonBuilder: Introduce typed typebarriers, r=jandem

Comment 3

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/b31bfbb2bdc4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130403 Firefox/23.0 ID:20130403083520
Bad:
http://hg.mozilla.org/mozilla-central/rev/b5cb88ccd907
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130403 Firefox/23.0 ID:20130403084327
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b31bfbb2bdc4&tochange=b5cb88ccd907


Triggered by:
Merge baseline compiler...

Comment 4

[Hannes Verschore [:h4writer]]

If memory serves well, I don't think bug 909717 would/can solve a bug. I think the changes probably hide the original problem... So we might want to look closer about what is happening...

Comment 5

[Jakub Galgonek]

If I replace call "new Vector_0()" by "new Vector_1(0.0, 0.0, 0.0)", then the code works fine on firefox/24.0. It also works, if I add "this.x_0 = 0.0; this.y_0 = 0.0; this.z_0 = z_0;" into Vector_0. Thus, the reported problem is observed, if vector constructed by Vector_0 uses x_0, y_0 and z_0 from its prototype.

Comment 6

[Jan de Mooij [:jandem]]

I'm looking into this now (ESR24 build).

There are 2 different MCreateThisWithProto instructions, each is followed by its own MBox and somehow GVN thinks the two MBox instructions have the same operand. Will know more soon.

Comment 7

[Jan de Mooij [:jandem]]

Ugh, MCreateThisWithProto inherits congruentTo from MBinaryInstruction :(

Comment 8

[Jan de Mooij [:jandem]]

Jakub, thank you for the report. This is an older IonMonkey bug, surprised it doesn't break more stuff.

This needs some refactoring to avoid similar bugs in the future, but I'll post simpler patches for aurora/beta/ESR.

Comment 9

[Jan de Mooij [:jandem]]

Although this testcase does not fail on Firefox 26+, the problem is still there. Updating flags.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

The main problem is that MBinaryInstruction and friends shouldn't override congruentTo: it means all instructions that inherit from them will get a default implementation that you probably don't want or expect. In this case MCreateThisWithProto but there are others I think.

So this patch:

(*) Removes MTernaryInstruction::congruentTo and MQuaternaryInstruction::congruentTo. Updated derived classes to call congruentIfOperandsEqual.

(*) Renames MBinaryInstruction::congruentTo to binaryCongruentTo. It's like congruentIfOperandsEqual, but also handles commutative instructions.

(*) Fixes some classes that inherit from these. For effectful instructions, we don't need to override congruentTo (congruentIfOperandsEqual will always return false). For the other classes, I made sure they override congruentTo.

(*) Fixes some congruentTo issues I noticed.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 0 - Fix BinaryBitwiseInstruction

The previous patch breaks the testBullet.js jit-test, because we mark MBinaryBitwiseInstruction as commutative when we specialize it as int32. With the patch binaryCongruentTo takes advantage of this (we didn't do this before) and things break because shift instructions are not commutative.

The fix is to only mark MBinaryBitwiseInstruction as commutative if it's a BitOr, BitAnd or BitXor.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8394345 [details] [diff] [review]
Patch

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

I think you have forgotten to add congruentTo to the following ops:
MAtan2
MHypot
MPow
MConcat
MSetArgumentsObjectArg (maybe?)
MToId
MStringSplit

There are even more that inherited MBinaryInstruction::congruentTo, but I don't think they need congruentTo.

::: js/src/jit/MIR.cpp
@@ +170,5 @@
>  
>      if (isEffectful() || ins->isEffectful())
>          return false;
>  
> +    MOZ_ASSERT(numOperands() == ins->numOperands());

I think we might want to keep the numOperands() != ins->numOperands() check.
E.g. for a MInstruction inheriting MVariadicInstruction. We currently don't have such a case, but I don't think it makes sense to disable it. At least I don't see a reason for it.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> I think you have forgotten to add congruentTo to the following ops:
> MAtan2
> MHypot
> MPow
> MConcat

These all override congruentTo.

> MSetArgumentsObjectArg (maybe?)
> MToId

These are effectful so we'll never eliminate them.

> MStringSplit

For this one we don't want congruentTo, it can cause bugs like the one here: two similar MStringSplit instructions have to return different arrays.

> I think we might want to keep the numOperands() != ins->numOperands() check.
> E.g. for a MInstruction inheriting MVariadicInstruction. We currently don't
> have such a case, but I don't think it makes sense to disable it. At least I
> don't see a reason for it.

OK I can add it back.

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f11569eb914c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7088a1f186

Will prepare simpler patches for other branches.

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> These all override congruentTo.

Indeed. My bad.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/f11569eb914c
https://hg.mozilla.org/mozilla-central/rev/ba7088a1f186

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Patch for branches

Very simple and safe fix for branches.

Comment 18

[Hannes Verschore [:h4writer]]

Comment on attachment 8396713 [details] [diff] [review]
Patch for branches

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

Nice to have this fixed for these two issues. Definitely correct.
Now since we now this is a bigger issue, which metric did you use to decide on what was important to uplift and what not?

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #18)
> Now since we now this is a bigger issue, which metric did you use to decide
> on what was important to uplift and what not?

I fixed all instructions that inherit from BinaryInstruction and have this problem. The m-c patch is better because it prevents similar bugs in the future, but the other patch should fix the problems on other branches.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8396713 [details] [diff] [review]
Patch for branches

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Older Ion bugs.
User impact if declined: Broken websites, like the one reported here
Testing completed (on m-c, etc.): A similar patch landed on m-c more than a week ago.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76754e144866
https://hg.mozilla.org/releases/mozilla-beta/rev/81285325c7db

Comment 22

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on Firefox 24RC and verified as fixed on Firefox 29 beta 7, latest Aurora and latest Nightly using Ubuntu 13.04 64bit.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Jan - Given that this issue was reported on ESR 24.3.0 and your branch patches are very small, what is your opinion on the risk associated with backporting this fix to ESR24?

Comment 24

[Hannes Verschore [:h4writer]]

(In reply to Lawrence Mandel [:lmandel] from comment #23)
> Jan - Given that this issue was reported on ESR 24.3.0 and your branch
> patches are very small, what is your opinion on the risk associated with
> backporting this fix to ESR24?

Oh. This can easily get nominated for esr24 uplift. Should be very low risk.

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Sounds good. If you can get a branch patch created or the esr24 approval flag set to nom for the existing patch I'll follow up with approval. If we can get this landed this week it can get into ESR 24.6.0.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8396713 [details] [diff] [review]
Patch for branches

Lawrence, I've been told in the past that we're only fixing security issues on ESR24 at this point. Requesting approval in case that's not true.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Correctness issues.
User impact if declined: Broken websites.
Fix Landed on Version: 31, backported to 29
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8396713 [details] [diff] [review]
Patch for branches

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

ESR approval granted. Please land this week.

The general rule is that ESR is for high severity security fixes. However, we do have the ability to ship other fixes, specifically correctness fixes. In this case the fix was reported by an ESR user on the ESR channel and is a low risk fix.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

I dropped the MStringSplit hunk since bug 935016 only landed on 28+.

https://hg.mozilla.org/releases/mozilla-esr24/rev/4a99027b4a02