Wrong results if JIT is enabled

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jakub.galgonek, Assigned: jandem)

Tracking

23 Branch
mozilla31
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 wontfix, firefox28 wontfix, firefox29 verified, firefox30 verified, firefox31 verified, firefox-esr2430+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(4 attachments)

Reporter

Description

5 years ago
Posted file 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.
Assignee

Comment 1

5 years ago
We should bisect this probably.
Flags: needinfo?(jdemooij)

Comment 2

5 years ago
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
Status: UNCONFIRMED → NEW
Depends on: 909717
Ever confirmed: true

Comment 3

5 years ago
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...
Version: 24 Branch → 23 Branch
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...
Reporter

Comment 5

5 years ago
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.
Assignee

Comment 6

5 years ago
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.
Assignee

Comment 7

5 years ago
Ugh, MCreateThisWithProto inherits congruentTo from MBinaryInstruction :(
Assignee

Comment 8

5 years ago
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Assignee

Comment 9

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

Comment 10

5 years ago
Posted patch PatchSplinter Review
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.
Attachment #8394345 - Flags: review?(hv1989)
Assignee

Comment 11

5 years ago
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.
Attachment #8394379 - Flags: review?(hv1989)
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.
Attachment #8394345 - Flags: review?(hv1989) → review+
Attachment #8394379 - Flags: review?(hv1989) → review+
Assignee

Comment 13

5 years ago
(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.
(In reply to Jan de Mooij [:jandem] from comment #13)
> These all override congruentTo.

Indeed. My bad.
https://hg.mozilla.org/mozilla-central/rev/f11569eb914c
https://hg.mozilla.org/mozilla-central/rev/ba7088a1f186
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Updated

5 years ago
Flags: needinfo?(jdemooij)
Assignee

Comment 17

5 years ago
Very simple and safe fix for branches.
Attachment #8396713 - Flags: review?(hv1989)
Flags: needinfo?(jdemooij)
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?
Attachment #8396713 - Flags: review?(hv1989) → review+
Assignee

Comment 19

5 years ago
(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.
Assignee

Comment 20

5 years ago
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.
Attachment #8396713 - Flags: approval-mozilla-beta?
Attachment #8396713 - Flags: approval-mozilla-aurora?
Attachment #8396713 - Flags: approval-mozilla-beta?
Attachment #8396713 - Flags: approval-mozilla-beta+
Attachment #8396713 - Flags: approval-mozilla-aurora?
Attachment #8396713 - Flags: approval-mozilla-aurora+
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.
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?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
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.
Assignee

Comment 26

5 years ago
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.
Attachment #8396713 - Flags: approval-mozilla-esr24?
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.
Attachment #8396713 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
You need to log in before you can comment on or make changes to this bug.