Closed Bug 1132290 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving Uint8Array

Categories

(Core :: JavaScript Engine: JIT, defect, major)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

f = function() {
    v = Uint8Array()
    function f(x) {
        print(x + v[0] | 0)
    }
    return f
}()
f(0)
f(1)

$ ./js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e --fuzzing-safe --no-threads --ion-eager testcase.js
0
1

$ ./js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis --no-sse3 testcase.js
0
0

Tested this on m-c rev 38058cb42a0e.

My configure flags are:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-optimize --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-optimize --enable-more-deterministic --enable-nspr-build --32 -R ~/trees/mozilla-central" -r 38058cb42a0e

This goes back beyond http://hg.mozilla.org/mozilla-central/rev/03c6a758c9e8, will see if I can get a better bisection range. Till then, setting needinfo? from Hannes and Jan as a start.
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
This goes back before end-2013 too. ( https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3 )
Another case of truncation going wrong.

> MLoadTypedArrayElementStatic 0
> MAdd 100 MLoadTypedArrayElementStatic
> MBitOr MAdd 0
> MPrint MBitOr

becomes

> MLoadTypedArrayElementStatic 0 (infallible)
> MAdd 100 MLoadTypedArrayElementStatic
> MTruncate MAdd
> MPrint MTruncate

Where the "MLoadTypedArrayElementStatic" is now infallible, because the propagation of truncation.

MLoadTypedArrayElementStatic is typed int32, fallible with undefined
MAdd is typed int32, fallible with double

I said there was a hole in truncation, because it doesn't take the hidden path (bailout path) into consideration when deciding to truncate.

If only looking at the uses of MLoadTypedArrayElementStatic we can indeed say all uses get truncated.
This is incorrect for the bailout path:

(undefined + 100) | 0 => 0
is not equal to:
undefined | 0 + 100 | 0 => 100

Solutions:

- The fix in bug 1130679 for the truncation error was only when "UseRemoved". This example wants "disable all truncation on fallible functions", which would cause a lot of !regressions!, but would fix all similar truncation bugs

- Annotate MLoadTypedArrayElementStatic as not truncatable, would fix this bug. But might be not enough. In this case we should look at all instructions if they do something similar.

I think the regression range is caused by:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9427895561
Blocks: 864214
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Flags: needinfo?(bhackett1024)
Oh sorry. I think I didn't make myself clear enough. I will probably post a patch to do solution 2 soonish ;). Still checking some random bits to make sure it is enough.
Assignee: nobody → hv1989
Flags: needinfo?(bhackett1024)
Just requesting feedback from Brian so he has an idea I'm removing this + a question if something should get checked. Any particular cases that might not get optimized now that should get optimized?
Attachment #8563342 - Flags: review?(nicolas.b.pierron)
Attachment #8563342 - Flags: feedback?(bhackett1024)
Comment on attachment 8563342 [details] [diff] [review]
Remove truncation of MLoadTypedArrayElementStatic

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

Do the following modification, and r=me.

::: js/src/jit/RangeAnalysis.cpp
@@ -2517,5 @@
>  bool
> -MLoadTypedArrayElementStatic::needTruncation(TruncateKind kind)
> -{
> -    if (kind >= IndirectTruncate)
> -        setInfallible();

Just change the condition to:
  kind == Truncate

and comment that:
  This instruction is fallible as we need to check if the index is readable, in which case we might have to bailout to return "undefined".  When this instruction is explicitly truncated, then we can just return "0" instead of undefined and stay in Ion.

Note that as "undefined" is not a numerical value which can be truncated linearly, we cannot use IndirectTruncate to remove this bailout.
Attachment #8563342 - Flags: review?(nicolas.b.pierron) → review+
Oh right. We can still do the explicit truncation. Awesome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/848ee095492d
Attachment #8563342 - Flags: feedback?(bhackett1024)
https://hg.mozilla.org/mozilla-central/rev/848ee095492d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.