1130618 - Crash at SIGTRAP with testcase involving SIMD

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

// Randomly chosen test: js/src/jit-test/tests/asm.js/bug1126251.js
function asmCompile() {
    return Function.apply(null, arguments);
}
function asmLink(f) {
    return f.apply(null, Array.slice(arguments, 1));
}
asmLink(asmCompile('global', `
    "use asm";
    var float32x4 = global.SIMD.float32x4;
    var splat = float32x4.splat;
    function e() {
        splat(.1e+71);
    }
    return e;
`), this)();

crashes js debug and opt shell on m-c changeset aa5f8d47a0ba with --fuzzing-safe --no-threads --ion-eager --ion-gvn=off --ion-check-range-analysis at SIGTRAP.

Debug configure options:

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

Opt configure options:

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8497232f9bb2
user:        Benjamin Bouvier
date:        Wed Jan 28 10:24:06 2015 +0100
summary:     Bug 1126251: Force float32 coercion of Float AsmJSNumLit input; r=luke

Benjamin, is bug 1126251 a likely regressor?

Comment 1

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

Setting s-s to be safe since this is a SIGTRAP.

Comment 2

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

Attachment: opt stack

(lldb) bt
* thread #1: tid = 0x2ea6c, 0x00000001017fd093, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x00000001017fd093
(lldb) dis -p
-> 0x1017fd093:  movsd  0x157d(%rip), %xmm2
   0x1017fd09b:  ucomisd %xmm1, %xmm1
   0x1017fd09f:  jp     0x1017fd0b0
   0x1017fd0a5:  ucomisd %xmm2, %xmm1
(lldb) register read $rip
     rip = 0x00000001017fd093
(lldb)

Again, not entirely sure if 0x00000001017fd093 is truly implicated here.

Comment 3

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

Attachment: debug stack

(lldb) bt
* thread #1: tid = 0x2ea19, 0x0000000101bf7093, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x0000000101bf7093
    frame #1: 0x000000010020ab92 js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba`js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [inlined] js::CallJSNative(native=0x00000001000a0c60)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 74 at jscntxtinlines.h:226
    frame #2: 0x000000010020ab48 js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba`js::Invoke(cx=0x0000000101e022d0, args=CallArgs at 0x00007fff5fbfe610, construct=<unavailable>) + 808 at Interpreter.cpp:498
    frame #3: 0x00000001001ed1b3 js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba`js::Invoke(cx=0x0000000101e022d0, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 787 at Interpreter.cpp:554
    frame #4: 0x0000000100453eea js-dbg-64-dm-nsprBuild-darwin-aa5f8d47a0ba`js::jit::DoCallFallback(cx=0x0000000101e022d0, frame=<unavailable>, stub_=0x0000000101e28a10, argc=0, vp=0x00007fff5fbfea58, res=<unavailable>) + 1930 at BaselineIC.cpp:9514
    frame #5: 0x0000000101bedd0b
(lldb)

Comment 4

[Christian Holler (:decoder)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)

> (lldb) register read $rip
>      rip = 0x00000001017fd093
> (lldb)
> 
> Again, not entirely sure if 0x00000001017fd093 is truly implicated here.

Please look at the previous instruction. If it's an int3, then the instruction you're displaying here is irrelevant and never executed.

Comment 5

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

(In reply to Christian Holler (:decoder) from comment #4)
> Please look at the previous instruction. If it's an int3, then the
> instruction you're displaying here is irrelevant and never executed.

I would love to if I could switch to that frame, assuming you're referring to the line "0x1017fd09b:  ucomisd %xmm1, %xmm1". How can I inspect $xmm1?

Process 47466 stopped
* thread #1: tid = 0x54e4d, 0x00000001017fd093, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
    frame #0: 0x00000001017fd093
-> 0x1017fd093:  movsd  0x157d(%rip), %xmm2
   0x1017fd09b:  ucomisd %xmm1, %xmm1
   0x1017fd09f:  jp     0x1017fd0b0
   0x1017fd0a5:  ucomisd %xmm2, %xmm1
(lldb) f 1
error: Frame index (1) out of range.
(lldb) bt
* thread #1: tid = 0x54e4d, 0x00000001017fd093, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x00000001017fd093
(lldb)

There seems to only be one line in the opt stack.

So, not sure what should be looked out for here.

Comment 6

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

:decoder mentioned over IRC that I should try this:

(lldb) dis --start $pc-100 --end $pc
   0x101bf702f:  orl    $0x1624, %eax
   0x101bf7034:  cvtss2sd %xmm1, %xmm1
   0x101bf7038:  movsd  0x15f8(%rip), %xmm2
   0x101bf7040:  ucomisd %xmm2, %xmm1
   0x101bf7044:  jae    0x101bf704b
   0x101bf704a:  int3
   0x101bf704b:  xorpd  %xmm2, %xmm2
   0x101bf704f:  ucomisd %xmm2, %xmm1
   0x101bf7053:  jne    0x101bf7076
   0x101bf7059:  jp     0x101bf7076
   0x101bf705f:  movsd  0x15d9(%rip), %xmm2
   0x101bf7067:  divsd  %xmm1, %xmm2
   0x101bf706b:  ucomisd %xmm1, %xmm2
   0x101bf706f:  ja     0x101bf7076
   0x101bf7075:  int3
   0x101bf7076:  movsd  0x15ca(%rip), %xmm2
   0x101bf707e:  ucomisd %xmm1, %xmm1
   0x101bf7082:  jp     0x101bf7093
   0x101bf7088:  ucomisd %xmm1, %xmm2
   0x101bf708c:  jae    0x101bf7093
   0x101bf7092:  int3

So this is probably the int3 that was referred to.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

The assertRangeF code for float32 is unfortunately wrong.  Simply converting the float32 input to a double doesn't guarantee that the comparisons will be correct (in this case: a number which can't be represented as a float32 (rounded to +inf) is compared with the exact double representation of this same number).

There are two ways to fix this:
- either have a special code path for emitting assertRangeF, which does pretty much the same as emitting double checks, but converts all double constants to float32 constants before comparing.
- float32-truncate the boundaries of range checks, if the input has a float32 type.  That sounds better, as it's equivalent to the first one, but done at compile-time.

Could somebody open up this bug, please? (i'm not part of the security core group) The problem here is that the assertion code is wrong, there's no real danger.

Comment 8

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

Opening up as per comment 7.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Explicitly coerce double inputs to float32, when they flow into float32 SIMD operators

An issue quite similar to bug 1126251: we store a big double in a Value, then
say "this value is a float32" without coercing the double inside the Value.
Then the value obtains a precise double range, while the equivalent float32
value is +infinity, and range analysis checks get crazy.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Remove float32 range assertions, until bug 930697 is fixed

It doesn't quite make sense to have range asserts for float32 at the
moment, as range analysis isn't implemented for float32.  The only case where a
Float32 typed MIR node can have a Range, is if it is a MConstant.

This patch ensures we never hit such a case by crashing during lowering of
MAssertRange if the input type is a float32.  Dan, selecting you as a potential
reviewer as you're assigned on bug 930697, which is mentionned in the TODOs
here.

Not sure we do want this patch right now, actually, as these assert checks
revealed a few subtle bugs in AsmJS.  I would be ok with rejecting this patch,
but I'll listen to your opinion first.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: asmjs-coerce-float32.patch

Duh, bzexport seems to only work with mq, sorry for the mess.

Luke, please look at comment 9 for details.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=385fdd7b240d

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8562725 [details] [diff] [review]
asmjs-coerce-float32.patch

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5403,5 @@
>  
>              AsmJSNumLit doubleLit = ExtractNumericLiteral(f.m(), arg);
>              MOZ_ASSERT(doubleLit.which() == AsmJSNumLit::Double);
> +            Value asFloat = Float32Value(doubleLit.toDouble());
> +            *def = f.constant(asFloat, Type::Float);

I think ExtractNumericLiteral might also have this problem if you have fround() of a doublelit that is out of float precision range.  I wonder if we should have f.constant() or the MConstant constructor due this normalization (preferably, I think, the latter since it is the essential pinchpoint).

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: asm.patch

That's right.  I considered doing this at some point, but thought that all paths were clear.  At least this should prevent future bugs of the same category.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: ion.gvn.enable.patch

And some addendum to be able to disable GVN at runtime, so that we can test the same thing as in comment 1.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8563332 [details] [diff] [review]
asm.patch

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

Great, thanks.  Perhaps also add MOZ_ASSERT_IF(resultType() == MIRType_Float32, v.toDouble() == double(float(v.toDouble()))) in MConstant's constructor?

Comment 17

[Dan Gohman [:sunfish]]

Comment on attachment 8562724 [details] [diff] [review]
Remove float32 range assertions, until bug 930697 is fixed

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

Since it seems to have accidentally caught an actual bug, I'm inclined to leave this code in. It'll also be handy when we do get around to implementing float32 range analysis. If this code is obstructing progress, I'd be ok removing it too though.

Comment 18

[Dan Gohman [:sunfish]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> The assertRangeF code for float32 is unfortunately wrong.  Simply converting
> the float32 input to a double doesn't guarantee that the comparisons will be
> correct (in this case: a number which can't be represented as a float32
> (rounded to +inf) is compared with the exact double representation of this
> same number).

I don't see the bug here. All float32 values can be losslessly converted to double. There's no convertion *to* float32 here except in the code which converts the value that had previously been converted from float32, so we should never have a value which can't be represented as a float32.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Righto, I kept the float32 range analysis checks.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f503faaa2e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2a1a5c46d2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5f503faaa2e
https://hg.mozilla.org/mozilla-central/rev/9d2a1a5c46d2