1236114 - Differential Testing: Different output message involving Math.round

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return (!(Math.round(Math.hypot(Number.MIN_VALUE, Math.fround(x))) | 0) | 0) !== (Math.atanh(x) ? false : Math.tan(0))
}
f(Number.MIN_VALUE)
print(f(4294967295))

$ ./js-dbg-64-dm-darwin-c690c50b2b54 --fuzzing-safe --no-threads --ion-eager testcase.js
false

$ ./js-dbg-64-dm-darwin-c690c50b2b54 --fuzzing-safe --no-threads --baseline-eager testcase.js
true

Tested this on m-c rev c690c50b2b54.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

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

autoBisect is running.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/95e3a5cf6530
user:        ZongShen Shen
date:        Thu Jan 22 08:39:16 2015 -0800
summary:     Bug 1120069 - IonMonkey: Implement MTruncateToInt32 recover instruction. r=nbp

Nicolas, is bug 1120069 a likely regressor?

(I'd prefer for this to be slightly higher on your radar, it's not easy to ignore this bug)

Comment 2

[Nicolas B. Pierron [:nbp]]

RangeAnalysis incorrectly replaced  !(round(…) | 0)  by  !round(…)  assuming that  round(…)  always returns an Int32.  The BitOr is flagged as a recover instruction, but not used by any other recover instruction.  Thus it is removed by DCE, and not part of the Recover instruction pipeline, which only see the final expression  !round(…)  which assumes that Math.round always returns an Int32.

I think we should tell Range Analysis that  Math.round(…)  has a truncateAfterBailout behaviour, and make Range Analysis aware of the fact that the range of MRound if not actually truncated to an Int32.

Comment 3

[Nicolas B. Pierron [:nbp]]

Ok, the problem is simpler than what I thought.  This is not Range Analysis directly, but only the BitOps optimization made at the end of it.  (Which implies that we do not have to add Range Analysis info to fix non-Range Analysis bugs)

The problem is that we currently have something which remove unnecessary bit-ops, but it removes them sooner than Sink, which sinks unused instructions to Baseline side-exit.  As we have to keep the original semantic in bailout path, we should keep these bitops until we move these instructions to convert them to recover instructions.

One other issue of doing so, is that we have Effective Address Analysis (EAA) relies on the lack of BitOps for matching patterns, and replacing them by new one.  So we might have to either re-run DCE after EAA, or include a way to skip bitops in EAA.

Comment 4

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

I'm guessing this is related more to Math.round.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Move 'Sink' phase before the 'Remove Unnecessary Bitops' phase. r=

Comment 6

[Dan Gohman [:sunfish]]

Would it work to just run Sink before Range Analysis entirely?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #6)
> Would it work to just run Sink before Range Analysis entirely?

From what I recall we still do UCE based on Range Analysis result.
So, I would prefer not.

Comment 8

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> (In reply to Dan Gohman [:sunfish] from comment #6)
> > Would it work to just run Sink before Range Analysis entirely?
> 
> From what I recall we still do UCE based on Range Analysis result.
> So, I would prefer not.

Unless I'm misunderstanding, we do do UCE after range analysis. See the prepareForUCE function and its call in Ion.cpp. If range analysis made any blocks unreachable, Ion.cpp schedules a full GVN run, which includes UCE.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8704170 [details] [diff] [review]
IonMonkey: Move 'Sink' phase before the 'Remove Unnecessary Bitops' phase. r=

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

As discussed on irc, this isn't as scary as it first looks. The only shared state is the bitops array.

Comment 12

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98c9f66abd4365bd9b7fbd143d41c33ffe1967b4

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/548c0c1b881b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/548c0c1b881b

Comment 15

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

Nicolas, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch.

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #15)
> Nicolas, do you mind nominating this for approval‑mozilla‑esr45 as well? It
> would aid fuzzing on that branch.

By experience, IonMonkey is not well armored to support moving phases around.  Even if this change does not sounds armful, and I am confident that this should be ok, I prefer to keep it a week or two in central, such that fuzzers can find issues before we decide to backport this changes to any other branches.

(keeping the ni? as a reminder)

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

This never got nominated for uplift, do you still want to request it for aurora 46/ beta 47? We are heading into  the beta 11 build tomorrow morning.

Comment 18

[Nicolas B. Pierron [:nbp]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #17)
> This never got nominated for uplift, do you still want to request it for
> aurora 46/ beta 47? We are heading into  the beta 11 build tomorrow morning.

Isn't that beta 46 and aurora 47?
If there is only one cycle of beta, I will recommend against for the same reason as mentioned in comment 16.

I will nominate this for 47.

Comment 19

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8704170 [details] [diff] [review]
IonMonkey: Move 'Sink' phase before the 'Remove Unnecessary Bitops' phase. r=

Approval Request Comment
[Feature/regressing bug #]: Bug 1105574
[User impact if declined]: low, correctness issue.
[Describe test coverage new/current, TreeHerder]: 1 week.
[Risks and why]: Moving phases around in Ion is not something we have good assertions for in IonMonkey. (comment 16)
[String/UUID change made/needed]: N/A

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Nicolas. Wontfix for 46 but this should land in 47 aurora.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8704170 [details] [diff] [review]
IonMonkey: Move 'Sink' phase before the 'Remove Unnecessary Bitops' phase. r=

Regression fix, Aurora47+

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bde325bf8319