1085298 - Differential Testing: Different output message involving ternary constructs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Fix

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

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

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Fix

Totally correct. Yeah I still had to negate it. Though == is indeed better.

Comment 4

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c12ed538e44a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c12ed538e44a

Comment 6

[Jukka Jylänki]

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 7

[Hannes Verschore [:h4writer]]

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

Comment 8

[Jukka Jylänki]

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.

Comment 9

[Hannes Verschore [:h4writer]]

(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!

Comment 10

[Jukka Jylänki]

Ah, no worries, I understand now. Thanks!

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75158d5c8acd