1535482 - Assertion failure: !used(), at js/src/jit/Label.h:85 with --arm-asm-nop-fill=1

Status: RESOLVED FIXED

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

Description

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

Attachment: stack

The following testcase crashes on mozilla-central revision 9fa0296c189b (build with 'CC="clang -m32 -msse2 -mfpmath=sse"' PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig AR=ar 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' sh ./configure --target=i686-pc-linux --enable-simulator=arm --enable-debug --disable-profiling --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift, run with --fuzzing-safe --no-threads --no-baseline --no-ion --arm-asm-nop-fill=1 w555-out.wrapper w555-out.wasm):

See attachment.

Backtrace:

#0 js::jit::Label::~Label (this=0x1eb9) at js/src/jit/Label.h:85
#1 GenerateImportJitExit (masm=..., fi=..., funcImportIndex=5, offsets=0xda4, throwLabel=<optimized out>) at js/src/wasm/WasmStubs.cpp:1726
#2 js::wasm::GenerateStubs (env=..., imports=..., exports=..., code=0xf6593970) at js/src/wasm/WasmStubs.cpp:2216
#3 0x582f48d2 in js::wasm::ModuleGenerator::finishCodeTier (this=0xff858768) at js/src/wasm/WasmGenerator.cpp:1002
#4 0x582f5578 in js::wasm::ModuleGenerator::finishModule (this=0xff858768, bytecode=..., maybeTier2Listener=0x0) at js/src/wasm/WasmGenerator.cpp:1100
/snip

For detailed crash information, see attachment.

I'm unsure if this is wasm-specific or similar to bug 1519037 (non-wasm-specific). Locking s-s out of precaution.

Comment 1

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

Attachment: 555.tar.xz

Testcase

Comment 2

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/3d46648331c1
user: Lars T Hansen
date: Fri Mar 23 15:45:50 2018 +0100
summary: Bug 1445277 - Suppress GC when wasm is active and running with wasm-gc support. r=bbouvier

Lars, is bug 1445277 a likely regressor?

Comment 3

[Lars T Hansen [:lth]]

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

Lars, is bug 1445277 a likely regressor?

That's almost certainly a red herring, the bug is elsewhere.

Comment 4

[Lars T Hansen [:lth]]

I can repro.

Comment 5

[Lars T Hansen [:lth]]

This failure is caused by an out-of-range branch. (Huge chunk of code here, we're generating stubs for > 42K exports, and it's one of the last of these that causes the failure.) The error is signaled correctly - from what I can see - in Assembler::as_b(), which calls m_buffer.fail_bail(), but this results in a label in a wonky state, and thus we assert later along an error return path. Probably not s-s because failure is propagated as it should be, but I'll dig a little deeper.

Comment 6

[Lars T Hansen [:lth]]

I would say the label assertion logic is simply not set up to cope with this problem.

Comment 7

[Lars T Hansen [:lth]]

Ah, there's even a comment in as_b() saying this very thing:

    // This will currently throw an assertion if we couldn't actually
    // encode the offset of the branch.

Comment 8

[Lars T Hansen [:lth]]

Attachment: Bug 1535482 - Reset a label along an error path. r?bbouvier

There is error checking logic for labels that is correct and desirable
in most cases, but it does not work well when the compilation aborts
due to other errors than OOM. In particular, ARM has code range
limits that can cause the assembler to abort when very large programs
are compiled. In these cases, labels that are live and have been
referenced but not bound (because the compilation did not get that
far) will cause assertions.

A particular case is the wasm stubs code, which generates a very large
chunk of code (if there are enough stubs to generate) where there are
many forward references to a label (for "throw") that is bound at the
very end of the generation process. On ARM, any sufficiently large
program (in the number of exports, say) will cause an assertion here.

We can fix this locally by resetting the label on the way out in a
scope exit function; effectively this disables the particular error
checking for this one label.

Comment 9

[Lars T Hansen [:lth]]

I should note that what happens next is that the shell terminates with OOM.

Basically, this situation is not good. For JS it's one thing to OOM here, it can fall back on the interpreter in the worst case. But for wasm we compile the program or we don't run at all (modulo tier-2 falling back to tier-1).

And the fact that the label assertion logic does not check bail() is arguably the underlying bug. We could try to propagate that up to the context, I guess... Not sure how that would look.

Comment 10

[Lars T Hansen [:lth]]

I've removed review here for the time being because (a) this is not a time-sensitive s-s bug, the assertion is essentially benign, and (b) we have a different problem to fix here.

The problem we should be fixing is that there can be a volume of stubs code exceeding the max branch distance (32MB). I expect this can be true even without nopfill. We should consider whether we can mitigate this by inserting jump islands into the generated code at appropriate locations so that we don't go out of range for the throwLabel. I don't think this should be very hard or very expensive; the hard part is testing that it works properly...

Comment 11

[Lars T Hansen [:lth]]

The problem here is a suboptimal fix that came in with bug 919592, this landed almost precisely five years ago today. The problem that that bug was trying to fix was to avoid placing non-representable instruction offsets into forward branch instructions that reference already-used labels, see bug 919592 comment 4. Thus it checks whether the offset of the previous instruction referencing the branch (which is the offset that needs to be recorded in the next branch) is representable as a branch offset, and bails out otherwise.

The bind() code has a similar check.

In practice, this bounds the size of the assembler code buffer by creating a maximum allowable offset, it just does so in a really obscure way. It would probably be better to enforce this limitation in the buffer itself. We can keep the checks that are in as_bl() and as_b() but we should really beef up the comments. It's possible the comments were made obscure so as to avoid exposing security information at the time.

But if we do bound the buffer this way we should be concerned about whether large wasm modules can be compiled with our current ARM back-end at all, as the maximum offset is 32MB. It's possible parallel compilation + far jump islands help here, and that we're only seeing this problem because of --no-threads. If so, it is possible we should be batching compilations even in single-threaded mode.

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1535482 - Limit the assembler buffer size to encodable offsets. r=luke

See extensive comment in the patch for more information.

Comment 13

[Lars T Hansen [:lth]]

Removing r? again because I am thoroughly going to boil this ocean.

Comment 14

[Lars T Hansen [:lth]]

Hm, patch is suboptimal because it also limits the size of the final pasteup assembly to this amount, not just each individual compilation task.

Comment 15

[Lars T Hansen [:lth]]

Pulling review again to address that, but we'll get there one day...

A hacky test to allow that buffer to be larger is promising, at that point we run into far-jump island problems, which is probably good. I have some prototype code to deal with that too but there's more to do there.

Comment 16

[Lars T Hansen [:lth]]

The order of tasks is:

• generate code for the program, and only add far jump islands after a task if we have a lot of code
• generate stubs
• link the compiled code, adding any necessary far jump islands

If the amount of stubs code is large [YMMV but here it's > 20MB] there's a real risk that far jump islands can't be added during the final link because the stubs code has created too great a distance between the program's code and its islands.

So it looks like we should treat stub code as the output of a compilation task, doing some linking before we paste it up if necessary.

And of course bug 1536105 notes that we should deal with the case where the amount of stubs code is > 32MB too, and the stubs code has to be split into multiple pieces effectively.

In addition, the assembler buffer size is currently limited to the branch offset distance, but this may be too lenient because it makes it likely that if we have a full buffer and try to add far jump islands after it during linking, we will fail to reach some of those islands. So possibly we should drop down to 20MB (on ARM) as for the InRange() function. Really the number we want is approximately 32MB - (number of calls that might have to be patched * size of far jump island) but unless we feel strongly that we should support single functions larger than 20MB of machine code this is hair-splitting.

Comment 18

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

Backed out for nojit build bustages in MacroAssembler.h:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e2c516df2f5b3d464a4523e1b8a7520c2369a3

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=235826575&repo=mozilla-inbound

/builds/worker/workspace/build/src/js/src/jit/MacroAssembler.h:3204:26: error: 'setUnlimitedBuffer' was not declared in this scope

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/320b3be39f9f96014157d09cee445e5dd6b64f26
https://hg.mozilla.org/mozilla-central/rev/320b3be39f9f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies Beta backport consideration or can this just ride the trains?

Comment 21

[Lars T Hansen [:lth]]

No, per comment 10 this is not really s-s (it's just part of a cluster of bugs that's s-s) and any manifestations are mostly debug-mode only, so we should let it ride the trains.

Comment 22

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

Attachment: 555.wasm

555.wrapper:

binary = read(scriptArgs[0], 'binary');
new WebAssembly.Instance(new WebAssembly.Module(binary));

Compile with --enable-simulator=arm --enable-debug --enable-more-deterministic and other 32-bit configuration parameters, on m-c rev f8e2c516df2f

./js-dbg-32-dm-armsim32-linux-x86_64-f8e2c516df2f --fuzzing-safe --no-threads --no-baseline --no-ion --arm-asm-nop-fill=1 555.wrapper 555.wasm

(Posting this after running the testcase through wasm-reduce, for future reference)