1006910 - Differential Testing: Different output message involving Math.asinh

Status: RESOLVED DUPLICATE of bug 1007213

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return (-(~undefined >>> 0) - Math.asinh(x) >> 0)
}
f(0)
print(f(1))

$ ./js-opt-32-dm-ts-linux-7dcb8e0d9668 --fuzzing-safe --ion-parallel-compile=off testcase.js
1

$ ./js-opt-32-dm-ts-linux-7dcb8e0d9668 --fuzzing-safe --ion-parallel-compile=off --ion-eager testcase.js
0

(Tested this on 32-bit Linux js opt threadsafe deterministic shell off m-c rev 7dcb8e0d9668)

My configure flags are:

CC="gcc -m32 -msse2 -mfpmath=sse" CXX="g++ -m32 -msse2 -mfpmath=sse" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/348b2ba27515
user:        David Caabeiro
date:        Mon Jul 15 10:03:14 2013 -0500
summary:     Bug 717379, part 1 - Implement the new ES6 math functions. r=jorendorff.

This seems to go way back to rev 348b2ba27515, however, Hannes, do you mind looking at this? (not sure if David will be the right person)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

I have an easy workaround for that, but the right and correct solution is in bug 1007213

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test

I thought the easy workaround was to inline the asinh math in inlineCallNative, as inlined math functions return doubles (and subsequent operations would have been carried out with doubles). However, it should be inlined, but because of a typo in inlineCallNative, it wasn't (until bug 1010286's patch, which doesn't provoke inlining though, as the observed return typeset only contains Int32).

The correct fix for this kind of issues is bug 1007213. In the meanwhile, we can add some GVN so that:
1) TruncateToInt32(undefined) is 0 (as per spec).
2) Ursh(constant, constant) is replaced by its value.
This way, the ursh is replaced by a double value and subsequent operations are done with doubles.

It looks like a rather unfortunate workaround before a fix in bug 1007213, but I can't see anything else to do, other than that or dup the bug (if the latter case suits you better, feel free to cancel the review flag and close the bug).

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8422577 [details] [diff] [review]
Patch + test

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

Please create a new bug report for this. This is purely an improvement, not a fix!
It only hides the issue seen here...

Are you looking into bug 1007213 comment 1 the "easy fix"?

::: js/src/jit/MIR.cpp
@@ +1357,5 @@
>  }
>  
> +MDefinition *
> +MUrsh::foldsTo(TempAllocator &alloc, bool useValueNumbers)
> +{

We cover this already! See MBinaryBitwiseInstruction::foldsTo. So this is not needed.

@@ +1371,5 @@
> +    int32_t rhsv = rhs()->toConstant()->value().toInt32();
> +    uint32_t result = lhsv >> (rhsv & 31);
> +
> +    if (specialization_ == MIRType_Int32)
> +        return MConstant::New(alloc, Int32Value(result));

Note: This would have been incorrect for negative lhs and 0 as rhs! Since in that case the result will be unsigned int.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> Are you looking into bug 1007213 comment 1 the "easy fix"?
No I am not.

> 
> ::: js/src/jit/MIR.cpp
> @@ +1357,5 @@
> >  }
> >  
> > +MDefinition *
> > +MUrsh::foldsTo(TempAllocator &alloc, bool useValueNumbers)
> > +{
> 
> We cover this already! See MBinaryBitwiseInstruction::foldsTo. So this is
> not needed.
You're right, that makes this patch almost empty (just replacing TruncateToInt32(undefined) by 0), so I don't think it's really worth it.

> 
> @@ +1371,5 @@
> > +    int32_t rhsv = rhs()->toConstant()->value().toInt32();
> > +    uint32_t result = lhsv >> (rhsv & 31);
> > +
> > +    if (specialization_ == MIRType_Int32)
> > +        return MConstant::New(alloc, Int32Value(result));
> 
> Note: This would have been incorrect for negative lhs and 0 as rhs! Since in
> that case the result will be unsigned int.

This is actually correct ;) Ursh stands for unsigned right shift, so x >>> 0 will convert x into its unsigned form (it's actually used as the coercion for unsigned, in the asm.js type system).

Closing as dup of 1007213, which contains the general explanation and proposed solutions.