Closed Bug 1316830 Opened 3 years ago Closed 3 years ago

Differential Testing: Different output message involving addition

Categories

(Core :: JavaScript Engine: JIT, defect, P1, major)

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase, Whiteboard: [fuzzblocker])

Attachments

(2 files)

function f(x) {
    return (x | 0) - (-4294967295 | 0) + -2147483647
}
x = [1, 4294967295]
for (var j = 0; j < 2; ++j) {
    for (var k = 0; k < 3; ++k) {
        print(f(x[j], x[k]))
    }
}

$ ./js-dbg-64-dm-linux-d38d06f85ef5 --fuzzing-safe --no-threads --ion-eager testcase.js
-2147483647
-2147483647
-2147483647
-2147483649
-2147483649
-2147483649

$ ./js-dbg-64-dm-linux-d38d06f85ef5 --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis testcase.js
-2147483647
-2147483647
-2147483647
2147483647
2147483647
2147483647

Tested this on m-c rev d38d06f85ef5.

My configure flags are:

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-more-deterministic --enable-debug" -r d38d06f85ef5

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/07358be0ec02
user:        Sander Mathijs van Veen
date:        Tue Oct 11 07:04:00 2016 +0200
summary:     Bug 1295130 - Fold AddI opcode with constant into other AddI with constant r=nbp

Sander/Nicolas, is bug 1295130 a likely regressor? Setting as [fuzzblocker] because it is very hard to ignore an issue that seems to involve additions (AddI opcode).
Flags: needinfo?(sandervv)
Flags: needinfo?(nicolas.b.pierron)
The problem was that the MAdd constructor was assuming that if we created a
new MAdd with a specialized type given as argument, then if this type is
Int32, then the MAdd is necessarily Truncated.

This changed the result of the operation by replacing a bailout by a
wrap-around underflow in the test case.

This patch adds a new constructor arguments to MAdd to give the missing
TruncateKind type, taken out of the addition which is being replaced.

(Note, I forgot to add the test case in this patch, I will do that locally
and push it with it with this patch)
Attachment #8810371 - Flags: review?(hv1989)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(sandervv)
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8810371 [details] [diff] [review]
FoldLinearArith: Do not set new additions as truncated by default.

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

::: js/src/jit/MIR.h
@@ +6800,5 @@
>              setCommutative();
>          }
>      }
>  
> +    MAdd(MDefinition* left, MDefinition* right, MIRType type, TruncateKind truncateKind)

A bit unfortunate to create a new ctor for this: could either the one right above take the truncateKind parameter with a default value (not sure this works with the TRIVIAL_NEW_WRAPPERS magic), or the above ctor defer to this one to reduce duplication?
Comment on attachment 8810371 [details] [diff] [review]
FoldLinearArith: Do not set new additions as truncated by default.

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

::: js/src/jit/MIR.h
@@ +6800,5 @@
>              setCommutative();
>          }
>      }
>  
> +    MAdd(MDefinition* left, MDefinition* right, MIRType type, TruncateKind truncateKind)

I think it would be best to remove the above definition for this one.
And create NewWasm with "lhs,rhs and type"-arguments that sets the correct truncateKind when type is Int32.
Attachment #8810371 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8810371 [details] [diff] [review]
> FoldLinearArith: Do not set new additions as truncated by default.
> 
> Review of attachment 8810371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MIR.h
> @@ +6800,5 @@
> >              setCommutative();
> >          }
> >      }
> >  
> > +    MAdd(MDefinition* left, MDefinition* right, MIRType type, TruncateKind truncateKind)
> 
> I think it would be best to remove the above definition for this one.
> And create NewWasm with "lhs,rhs and type"-arguments that sets the correct
> truncateKind when type is Int32.

We removed the NewWasm prefix in bug 1304672, so I'd rather have us not undo this, please :)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/321e451a82b2
FoldLinearArith: Do not set new additions as truncated by default. r=h4writer
Comment on attachment 8810416 [details] [diff] [review]
Use default value instead of duplicating code.

(r=bbouvier over irc)
Attachment #8810416 - Flags: review+
Keywords: checkin-needed
Attachment #8810371 - Flags: checkin+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2af053b06a
Use default value instead of duplicating code. r=bbouvier
Keywords: checkin-needed
[Tracking Requested - why for this release]:
This bug was introduced in FF52 and the patch should get uplifted to that release.
(In reply to Hannes Verschore [:h4writer] from comment #9)
> [Tracking Requested - why for this release]:
> This bug was introduced in FF52 and the patch should get uplifted to that
> release.

Based on hg annotation of changeset …

(In reply to Pulsebot from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/321e451a82b2
> FoldLinearArith: Do not set new additions as truncated by default. r=h4writer

Is already landed in 52, and is the patch fixing this issue. (landed with the test case)

(In reply to Pulsebot from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2af053b06a
> Use default value instead of duplicating code. r=bbouvier

Is landed in 53, but is only a clean-up for the previous patch.  It does not matter if we do not take it, but this would be nice to have.
https://hg.mozilla.org/mozilla-central/rev/321e451a82b2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No need to track. The important part apparently got into FF52!
You need to log in before you can comment on or make changes to this bug.