1524692 - Assertion failure: expect != replace && replace != output && output != expect, at js/src/jit/arm/MacroAssembler-arm.cpp:5333 with wasm

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

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

The following testcase crashes on mozilla-central revision d58901c5036f (build with --32 --enable-debug --enable-simulator=arm, run with --fuzzing-safe --no-threads --no-baseline --no-ion --ion-gvn=off w191-out.wrapper w191-out.wasm):

Backtrace:

#0  CompareExchange64<js::jit::BaseIndex> (masm=..., access=0xf66b90a8, sync=..., mem=..., expect=..., replace=..., output=...) at js/src/jit/arm/MacroAssembler-arm.cpp:5333
#1  js::jit::MacroAssembler::wasmCompareExchange64 (this=0xfffd45c8, access=..., mem=..., expect=..., replace=..., output=...) at js/src/jit/arm/MacroAssembler-arm.cpp:5384
#2  0x57f13383 in js::jit::CodeGenerator::visitWasmCompareExchangeI64 (this=0xfffd4d40, lir=0xf6314a90) at js/src/jit/arm/CodeGenerator-arm.cpp:3004
#3  0x57ffcee3 in js::jit::CodeGenerator::generateBody (this=0xfffd4d40) at js/src/jit/CodeGenerator.cpp:5964
#4  0x5802a596 in js::jit::CodeGenerator::generateWasm (this=0xfffd4d40, funcTypeId=..., trapOffset=..., offsets=0xfffd5900) at js/src/jit/CodeGenerator.cpp:10176
#5  0x582e5762 in js::wasm::IonCompileFunctions (env=..., lifo=..., inputs=..., code=0xf65d9d70, error=0xfffd6568) at js/src/wasm/WasmIonCompile.cpp:4091
#6  0x582d6960 in ExecuteCompileTask (task=<optimized out>, error=0xfffd6568) at js/src/wasm/WasmGenerator.cpp:713
/snip

For detailed crash information, see attachment.

Full configuration command with needed environment variables is:
PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig 'CC="clang -m32 -msse2 -mfpmath=sse"' AR=ar 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' sh /home/ubuntu/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-simulator=arm --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift

python3 -u -m funfuzz.js.compile_shell -b "--32 --enable-debug --enable-simulator=arm" -r d58901c5036f

Setting s-s as I don't know how bad this is. This seems restricted to ARM-specific shells only. Trying to get a bisection now...

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: Testcase

Comment 3

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

It's perhaps between the following:

Build breakage from:

changeset: https://hg.mozilla.org/mozilla-central/rev/284002382c21
user: Lars T Hansen
date: Thu Aug 24 15:17:58 2017 +0200
summary: Bug 1146817 - Make x86-shared/AtomicOperations-x86-shared-gcc.h 64-bit aware, and cleaner. r=sstangl

Build breakage was fixed:

changeset: https://hg.mozilla.org/mozilla-central/rev/05669ce25b03
user: Benjamin Bouvier
date: Tue Apr 03 18:13:51 2018 +0200
summary: Bug 1451038: Make clang compile the JS ARM32 simulator shell; r=froydnj

Not sure who's best to needinfo? so setting from :lth and :bbouvier.

Comment 4

[Lars T Hansen [:lth]]

I will take care of this.

Comment 5

[Lars T Hansen [:lth]]

--ion-gvn=off in any vanilla arm-simulator bug with DEBUG enabled is sufficient to repro.

Assuming this is a problem with register assignment constraints in code generation and not a problem with gvn, it will be sec-low since wasm atomics are disabled by default everywhere, but we'll see.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1524692-arm-atomic-reg-constraints.patch

This fixes the bug but I think I should go over the other ARM atomics to make sure they don't have similar problems.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1524692-arm-atomic-reg-constraints-v2.patch

Decided against a test case here since it would not catch problems reliably, plus we have good asserts to catch the problem in larger tests.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9041123 [details] [diff] [review]
bug1524692-arm-atomic-reg-constraints-v2.patch

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

Just to make sure I understand, any allocatable register pair would work?

One way to reduce the register pressure without dealing with the register allocator would be to constraint to non-overlapping random allocatable register pairs.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)

Comment on attachment 9041123 [details] [diff] [review]
bug1524692-arm-atomic-reg-constraints-v2.patch

Review of attachment 9041123 [details] [diff] [review]:

Just to make sure I understand, any allocatable register pair would work?

Yes, two cases: any random odd/even pair mostly, and any random two registers sometimes.

One way to reduce the register pressure without dealing with the register
allocator would be to constraint to non-overlapping random allocatable
register pairs.

Is there such a constraint available or would we have to add this to the logic somehow? Or do you mean, pick pairs at random during code generation and use fixed allocation as now?

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b040747c9287f333354c4adcfcc2efe8375880ff
https://hg.mozilla.org/mozilla-central/rev/b040747c9287

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 9041123 [details] [diff] [review]
bug1524692-arm-atomic-reg-constraints-v2.patch

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Users that enable SharedArrayBuffer for testing/developing threaded wasm content will experience mysterious failures on ARM hardware. It would be nice to keep this feature working, since we intend to ship it before long.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This fixes a possibly-incorrect register assignment (a missing constraint) by making it obviously-correct (if arguably overconstrained).

We actually have automated tests but they're never run because they're marked "slow"; I'm working on rectifying this.

String changes made/needed

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #9)

(In reply to Nicolas B. Pierron [:nbp] from comment #8)

One way to reduce the register pressure without dealing with the register
allocator would be to constraint to non-overlapping random allocatable
register pairs.

Is there such a constraint available or would we have to add this to the logic somehow? Or do you mean, pick pairs at random during code generation and use fixed allocation as now?

I meant picking a fixed allocation pair at random during the lowering.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9041123 [details] [diff] [review]
bug1524692-arm-atomic-reg-constraints-v2.patch

Assertion fix, verified in nightly, let's uplift for beta 7.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/909f45bf0ed3