1132290 - Differential Testing: Different output message involving Uint8Array

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

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.

Comment 1

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

This goes back before end-2013 too. ( https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3 )

Comment 2

[Hannes Verschore [:h4writer]]

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

Comment 3

[Hannes Verschore [:h4writer]]

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.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Remove truncation of MLoadTypedArrayElementStatic

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?

Comment 5

[Nicolas B. Pierron [:nbp]]

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.

Comment 6

[Hannes Verschore [:h4writer]]

Oh right. We can still do the explicit truncation. Awesome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/848ee095492d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/848ee095492d