Closed Bug 1103389 Opened 10 years ago Closed 9 years ago

Assertion failure: from->toArgument()->index() % SimdStackAlignment == 0, at jit/LIR.cpp

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 --- verified
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- verified
firefox-esr31 --- unaffected

People

(Reporter: gkw, Assigned: sunfish)

References

Details

(4 keywords)

Attachments

(2 files)

// Random chosen test: js/src/jit-test/tests/asm.js/simd-mandelbrot.js
(function(global) {
    "use asm"
    var toF = global.Math.fround
    var f4 = global.SIMD.float32x4
    function p(x, y, width, value, max_iterations) {
        x = x | 0
        y = y | 0
        width = width | 0
        value = value | 0
        max_iterations = max_iterations | 0
    }
    function m(xf, yf, yd, max_iterations) {
        xf = toF(xf)
        yf = toF(yf)
        yd = toF(yd)
        max_iterations = max_iterations | 0
        var _ = f4(0, 0, 0, 0), c_im4 = f4(0, 0, 0, 0)
        c_im4 = f4(yf, yd, yd, yf)
        d(c_im4)
    }
})()

asserts js debug shell on m-c changeset 7ab92d922d19 with --ion-eager --no-threads --ion-regalloc=lsra at Assertion failure: from->toArgument()->index() % SimdStackAlignment == 0, at jit/LIR.cpp.

Debug configure options:

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

This was found by combining random jit-tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/7ab92d922d19/js/src/jit-test/tests/asm.js/simd-mandelbrot.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4a1a4d119980
parent:      205834:ff2190c3dbfd
user:        Benjamin Bouvier
date:        Wed Sep 17 18:55:58 2014 +0200
summary:     Bug 1068331: Fix register allocation of SimdValueX4; r=sunfish

(Setting s-s because this involves LIR, but letting someone else set the sec-rating)

Benjamin, is bug 1068331 a likely regressor?
Flags: needinfo?(benj)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x34d75c, 0x0035548d js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::LMoveGroup::add(this=<unavailable>, from=<unavailable>, to=<unavailable>, type=<unavailable>) + 573 at LIR.cpp:538, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0035548d js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::LMoveGroup::add(this=<unavailable>, from=<unavailable>, to=<unavailable>, type=<unavailable>) + 573 at LIR.cpp:538
    frame #1: 0x0035b30e js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::LinearScanAllocator::reifyAllocations() [inlined] js::jit::VirtualRegister::ins(this=<unavailable>, this=0x028b52f8, this=0x003e5fde, ins=0x028b52f8, to=<unavailable>, type=FLOAT32X4) + 1182 at LinearScan.cpp:304
    frame #2: 0x0035b2c1 js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::LinearScanAllocator::reifyAllocations(this=0x003e5fde) + 1105 at LinearScan.cpp:389
    frame #3: 0x002edeb8 js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::LinearScanAllocator::go(this=0xbfffc168) + 392 at LinearScan.cpp:1316
    frame #4: 0x002edb2c js-dbg-opt-32-dm-nsprBuild-darwin-7ab92d922d19`js::jit::GenerateLIR(mir=0x028b4688) + 1660 at Ion.cpp:1647
(lldb)
What versions have the SIMD code? trunk only?
What happens if this value is unaligned?
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #2)
> What versions have the SIMD code? trunk only?
> What happens if this value is unaligned?

Only Nightly has SIMD activated, other versions are unaffected.

This bug shows up with --ion-regalloc=lsra (i.e. using the LSRA register allocator), which isn't used so far for SIMD code in the browser. In the browser, another register allocator (backtracking alloc) is used.

If a value is unaligned, in an opt build, the browser will crash (with SIGBUS) when trying to read or write the value.
Dan, any interest to work on this? I could take it on, but my knowledge of our regalloc is fairly low...
Flags: needinfo?(sunfish)
I'm traveling so I can't get to this right away, but I will take a look at this. Fortunately, going by comment 3, it seems that the conditions needed to trigger the bug don't yet occur in the wild.
This is similar to bug 1099216, but this time it's LSimdValueFloat32x4.
Assignee: nobody → sunfish
Flags: needinfo?(sunfish)
This is a similar fix, of disabling defineReuseInput, plus the additional twist that LSRA doesn't seem to handle the tempCopy correctly. I decided to just work around it and use a plain temp and manual copy for now. This is another motivation for the project to enable backtracking by default.
Flags: needinfo?(benj)
Attachment #8535789 - Flags: review?(benj)
Comment on attachment 8535789 [details] [diff] [review]
simdvalue-reuse.patch

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

Can't wait for BT to be enabled by default. Can you add the test case please?

::: js/src/jit/shared/Lowering-x86-shared.cpp
@@ +737,5 @@
>  {
>      if (ins->type() == MIRType_Float32x4) {
> +        // Ideally, x would be used at start and reused for the output, however
> +        // register allocation currently doesn't permit us to tie together two
> +        // virtual registers with different types.

Can you add a TODO and open up a bug, so that we don't forget to modify here (and the place in bug 1099216 as well) if/when BT gets enabled by default?
Attachment #8535789 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/3e6f63e57dc5
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Group: core-security
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx37
Is there a reason why this wasn't marked with any status (fixed or otherwise) for 38? I assume we fixed it there when it was fixed on 37 and 39?
Flags: needinfo?(gary)
Flags: needinfo?(sunfish)
(In reply to Christian Holler (:decoder) from comment #11)
> JSBugMon: This bug has been automatically verified fixed.
> JSBugMon: This bug has been automatically verified fixed on Fx37

(In reply to Al Billings [:abillings] from comment #12)
> Is there a reason why this wasn't marked with any status (fixed or
> otherwise) for 38? I assume we fixed it there when it was fixed on 37 and 39?

The versions were marked as above, perhaps one was missed.

Anyway, I checked that it was fixed for 38:

http://hg.mozilla.org/releases/mozilla-release/rev/3e6f63e57dc5
http://hg.mozilla.org/releases/mozilla-esr38/rev/3e6f63e57dc5
Flags: needinfo?(sunfish)
Flags: needinfo?(gary)
SIMD has been Nightly-only from the start.
You need to log in before you can comment on or make changes to this bug.