Closed Bug 1135047 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving Math.pow

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- affected
firefox39 --- fixed

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(3 files)

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.
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?
Flags: needinfo?(hv1989)
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).
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(hv1989)
Resolution: --- → DUPLICATE
Duplicate of bug: 1130679
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!
Status: RESOLVED → REOPENED
Depends on: 1130679
Resolution: DUPLICATE → ---
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?
Assignee: nobody → hv1989
Attachment #8569848 - Flags: feedback?(nicolas.b.pierron)
Blocks: 1137215
(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.
Attachment #8569848 - Flags: feedback?(nicolas.b.pierron)
Attached patch Minimal patchSplinter Review
Attachment #8570408 - Flags: review?(nicolas.b.pierron)
Sub-optimal: found two instructions that are still using the computed range instead of the output range.
Attachment #8570410 - Flags: review?(nicolas.b.pierron)
Attachment #8570408 - Flags: review?(sunfish)
Attachment #8570408 - Flags: review?(nicolas.b.pierron)
Attachment #8570408 - Flags: review+
Comment on attachment 8570410 [details] [diff] [review]
Optim: Use output range

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

Good catch!
Attachment #8570410 - Flags: review?(sunfish)
Attachment #8570410 - Flags: review?(nicolas.b.pierron)
Attachment #8570410 - Flags: review+
Comment on attachment 8570410 [details] [diff] [review]
Optim: Use output range

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

LGTM
Attachment #8570410 - Flags: review?(sunfish) → review+
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".
Attachment #8570408 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/15fc680d0c8b
https://hg.mozilla.org/mozilla-central/rev/7e8219a083f8
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.