Closed Bug 1085298 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving ternary constructs

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: gkw, Assigned: h4writer)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

function f(x, y) {
    return (y | 0 && x ? y | 0 : 0)
}
m = [1]
print(f(m[0], m[0]))
print(f(m[1], m[0]))
print(f(m[2], m[0]))

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --ion-eager testcase.js
1
0
1

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-33c0181c4a25 --fuzzing-safe --no-threads --baseline-eager testcase.js
1
0
0

Tested this on m-c rev 33c0181c4a25.

My configure flags are:

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-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6465e8acae5e
user:        Hannes Verschore
date:        Tue Sep 23 09:42:05 2014 +0200
summary:     Bug 1064537: IonMonkey: Try folding ternary constructs, r=nbp

Hannes, is bug 1064537 a likely regressor?
Flags: needinfo?(hv1989)
Attached patch Fix (obsolete) — Splinter Review
The comments explains what it should test. It is the test that is wrong. A branch should dominate explicitly 1 edge of the MPhi. Not 0, like is the case with this testcase.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8509794 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8509794 [details] [diff] [review]
Fix

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

::: js/src/jit/MIR.cpp
@@ +1269,3 @@
>          test->ifTrue()->dominates(block()->getPredecessor(1)))
>      {
>          return nullptr;

I think what you want is '==', or to negate this expression, in which case,
  if none are dominated, we fail
  if one is dominated, we continue
  if both are dominated, we fail
Attachment #8509794 - Flags: review?(nicolas.b.pierron)
Attached patch FixSplinter Review
Totally correct. Yeah I still had to negate it. Though == is indeed better.
Attachment #8509794 - Attachment is obsolete: true
Attachment #8510260 - Flags: review?(nicolas.b.pierron)
Attachment #8510260 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/c12ed538e44a
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I am getting this bug in the wild (in Unreal Engine 4) in current Firefox Aurora channel and spend some 9 hours yesterday and today bisecting the problem down to this bug. I am puzzled. This is a known bug that was introduced by https://hg.mozilla.org/mozilla-central/rev/6465e8acae5e and fixed by https://hg.mozilla.org/mozilla-central/rev/c12ed538e44a . The bug was correctly identified to affect Firefox 35 and 36 channels, but the fix was only applied to Firefox 36 channel. Why was this fix not uplifted? We will have a regressed JS engine going out in FF 35 channel otherwise!
Comment on attachment 8510260 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: bug 1064537
[User impact if declined]: Wrong results on when using ? :
[Describe test coverage new/current, TBPL]: Test added and on nightly for already a week
[Risks and why]: Not really a big risk. Small patch.
[String/UUID change made/needed]: None
Attachment #8510260 - Flags: approval-mozilla-aurora?
Thanks for the quick action Hannes! The related bug is this https://bugzilla.mozilla.org/show_bug.cgi?id=1093303 , however that might currently have extra security bits so it's not viewable outside the CC list, if I understand correctly.
(In reply to Jukka Jylänki from comment #6)
> I am getting this bug in the wild (in Unreal Engine 4) in current Firefox
> Aurora channel and spend some 9 hours yesterday and today bisecting the
> problem down to this bug. I am puzzled. This is a known bug that was
> introduced by https://hg.mozilla.org/mozilla-central/rev/6465e8acae5e and
> fixed by https://hg.mozilla.org/mozilla-central/rev/c12ed538e44a . The bug
> was correctly identified to affect Firefox 35 and 36 channels, but the fix
> was only applied to Firefox 36 channel. Why was this fix not uplifted? We
> will have a regressed JS engine going out in FF 35 channel otherwise!

Sorry, I totally lost track of this. I think normally the status-firefox36: should have been set to fixed. I get that message and in that case I would see status-firefox35 is still affected and request approval to uplift. This should definitely get fixed on aurora! Thanks for finding that this hasn't happened yet!
Ah, no worries, I understand now. Thanks!
Attachment #8510260 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.