1135047 - Differential Testing: Different output message involving Math.pow

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    print(Math.pow(x, x) !== Math.pow(x, x))
}
f(-0)
f(-undefined)

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

$ ./js-dbg-64-dm-nsprBuild-darwin-1b4c5daa7b7a --fuzzing-safe --no-threads --baseline-eager testcase.js
false
true

Tested this on m-c rev 1b4c5daa7b7a.

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

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 1b4c5daa7b7a

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/e5d631abcd56
user:        Tom Schuster
date:        Sun Oct 05 15:26:40 2014 +0200
summary:     Bug 1073576 - Optimize strict compares with equal operands. r=h4writer

Not sure if bug 1073576 being the regressor is entirely accurate. Hannes, thoughts?

Comment 2

[Hannes Verschore [:h4writer]]

It is debatable if bug 1073576 is the real cause. But by doing that we opened a floodgate of bugs. This is the same bug as bug 1130679. Actively looking into that.

(Btw thanks for all the different testcases. Giving me new ideas/ways to run into this).

Comment 3

[Hannes Verschore [:h4writer]]

After careful examination of the testcase, this is indeed partially bug 1130679.
But it also uncovers an additional bug.

MToInt32 (and others) are not recognized as filtering types, which it definitely does!
As a result the guards put in place to not remove this instruction are gone and we get this wrong result.

The issue is because we are working with two type of ranges:
1) Full range an instruction can output giving the range of the inputs (i.e. before bailouts)
2) Range of an instructions that uses only will see (i.e. after bailouts)

And the distinction between those two ranges are not always correct and done correctly.
e.g.
MToInt32
- range after computation before bailouts: input()->range()
- range after bailouts: intersection of input()->range() and MIRType_Int32

Currently MToInt32 gives the later range for both. So we don't recognize this instruction as filtering types!

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Patch: split inner and outer range

So the issue here is: MToInt32::computeRange which contains the range *after* bailouts.

- range() should contain the range *before* bailouts!
- (also some functions use "MDefinition::range()" instead of more specific Range(ins))

Honest mistakes.

So I propose:
-> adding "MDefinition::getOutputRange()" which should get used almost everywhere. Which contains the range *after* bailouts.
-> maybe changing "MDefinition::range()" to MDefinition::innerRange() ? s/computeRange/computeInnerRange/g
-> make it possible to override "MDefinition::getOutputRange()", so MToInt32 can adjust the range *after* bailouts.

This would allow other functions to override their *after* range. (Which e.g. MUrsh already wants to do).

This patch already fixes this bug.
What do you think?

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> So I propose:
> -> adding "MDefinition::getOutputRange()" which should get used almost
> everywhere. Which contains the range *after* bailouts.
> -> maybe changing "MDefinition::range()" to MDefinition::innerRange() ?
> s/computeRange/computeInnerRange/g
> -> make it possible to override "MDefinition::getOutputRange()", so MToInt32
> can adjust the range *after* bailouts.
> 
> This patch already fixes this bug.
> What do you think?

As Range Analysis is a complex beast which involve a lot of uplifts, (as you might discover), I would prefer if you can fix this the bad way first (by adding things in the Range constructor), and then make focus clean-up patches which are not changing the semantics.

To be honest, I like the look of the Range constructor syntax, as it is easy to distinguish from the pointer syntax, which simplifies the reviews a lot.  I think this would be fine if the Range constructor was initialized based on the returned value of the MDefinition::returnRange() function.

Also inner is to opposed to outer, and output is opposed to input.  I think just having range and returnedRange would be enough. I do not see the need for the renaming of the computeRange function.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Minimal patch

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Optim: Use output range

Sub-optimal: found two instructions that are still using the computed range instead of the output range.

Comment 8

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8570410 [details] [diff] [review]
Optim: Use output range

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

Good catch!

Comment 9

[Dan Gohman [:sunfish]]

Comment on attachment 8570410 [details] [diff] [review]
Optim: Use output range

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

LGTM

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8570408 [details] [diff] [review]
Minimal patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +1641,1 @@
>      setRange(output);

Please leave a comment here mentioning why we're not doing a clamp here. I guess something along the lines of: "MToInt32 enforces its int32 type in bailouts, so the pre-bailout range can be considered unclamped".

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15fc680d0c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8219a083f8

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15fc680d0c8b
https://hg.mozilla.org/mozilla-central/rev/7e8219a083f8