1317678 - Assertion failure: funcBeginToTableEntry_ == offsets.tableEntry - begin_, at js/src/wasm/WasmCode.cpp:390

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision c6e0621b8155+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug). To reproduce, you can run the following code in the JS shell (running with --wasm-always-baseline might be necessary):

var data = os.file.readFile(file, 'binary');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==8080==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000c56bce bp 0x7fffb0e81c50 sp 0x7fffb0e81bb0 T0)
    #0 0xc56bcd in js::wasm::CodeRange::CodeRange(unsigned int, unsigned int, js::wasm::FuncOffsets) js/src/wasm/WasmCode.cpp:389:5
    #1 0xd0c1c8 in void mozilla::detail::VectorImpl<js::wasm::CodeRange, 0ul, js::SystemAllocPolicy, true>::new_<unsigned int, unsigned int, js::wasm::FuncOffsets&>(js::wasm::CodeRange*, unsigned int&&, unsigned int&&, js::wasm::FuncOffsets&) dist/include/mozilla/Vector.h:171:7
    #2 0xd0c1c8 in bool mozilla::Vector<js::wasm::CodeRange, 0ul, js::SystemAllocPolicy>::emplaceBack<unsigned int, unsigned int, js::wasm::FuncOffsets&>(unsigned int&&, unsigned int&&, js::wasm::FuncOffsets&) dist/include/mozilla/Vector.h:623
    #3 0xd0c1c8 in js::wasm::ModuleGenerator::finishTask(js::wasm::IonCompileTask*) js/src/wasm/WasmGenerator.cpp:388
    #4 0xd257b7 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/wasm/WasmGenerator.cpp:960:14
    #5 0xc9fcae in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/wasm/WasmCompile.cpp:695:12
    #6 0xc9fcae in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:755
    #7 0xc9fcae in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:947
    #8 0xe86ee9 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:387:27
    #9 0x6056b0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5652:14
[...]

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Fails with baseline, not with Ion.

Comment 3

[Lars T Hansen [:lth]]

The problem disappears if I change the types of some fields in CodeRange from uint8_t to uint32_t: the values that needed to be stored were chopped.

The reason for the failure is possibly that the baseline compiler emits some prologue code at the end of the function and needs more space for saving some of these offsets.

Comment 4

[Lars T Hansen [:lth]]

Attachment: Disassembly of the test case

A very nasty little program.

Comment 5

[Lars T Hansen [:lth]]

Also a bug in the Ion back-end, but it needs a nastier test case...

What happens is that the test has a large number of local variables.  In the baseline compiler, these are all given homes on the stack, so the stack frame is going to be quite large (120KB, give or take).  The last lines of GenerateProfilingPrologue are:

   if (framePushed)
       masm.subFromStackPtr(Imm32(framePushed));

For large frames this will generate a sequence of stack probe instructions, enough to push masm.currentOffset() out of the uint8_t range (in the baseline compiler we end up with offset==448).  Hence that value, which is stored once GenerateProfilingPrologue returns, will later overflow the field in CodeRange which is a uint8_t.

Luke, any reason we can't increase the size of these fields?

Comment 6

[Luke Wagner [:luke]]

Oh goodness, that's quite a case.  Thanks for digging into it!

If we inflate all three funcBeginTo* uint8_ts to uint16_ts, then CodeRange will grow a word (and CodeRange contributes significantly to overall memory usage), so I'd rather just inflate funcBeginToTableEntry_ and express the other two as uint8_t offsets from the tableEntry.  It's all entirely encapsulated in the methods of CodeRange, so that shouldn't be too hard.

But I actually wonder whether we want to allow so many locals and such large stack frames in the first place.  MaxLocals (in WasmTypes.h) is 64k atm, but I wonder if it should be much smaller (like 4k).  One cause for concern is that baseline will always have O(num-locals) frame size but Ion will often have a much smaller frame and thus many-local functions could lead to cases where baseline jits quick exhaust stack space.  Because it happens at runtime, in the worst case, we could have a race condition between switching to Ion vs. hitting an over-recursed baseline error.  Thus clamping num-locals to something modest will encourage people to always "regalloc" their locals on the producer side which I think will be good for the web in general.

Comment 7

[Lars T Hansen [:lth]]

Currently baseline has a hard limit on the stack frame size at 256KB, this seemed sensible as a guard against attacks.  There's nothing magic about the number.  See bug 1282063 for some initial discussions around how to propagate a frame-just-too-darn-big error out of the compiler.

But I think we have another option here, which is that for large frames, instead of unrolling the stack probe loop we keep it as a loop.  That will make it possible for the prologue to always be "small", and we can stick with uint8_t for the fields of CodeRange without adding more complication and without impacting decisions elsewhere.  We should add assertions that the values for the fields of CodeRange don't overflow those fields.  I'll look into this.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1317678-reroll-stack-probe.patch

Something like this?  The cutoff - 8 pages - could be a symbolic constant, if that matters.  Another question is whether I should avoid the ScratchRegister on x64 and pull a trick like I have on x86.

Passes jit-tests locally and fixes the TC.

(Re your earlier comment, it makes sense to cap Wasm variable counts lower, but it may be good to land this patch regardless to make the code resilient.  Frames can be large for other reasons too.)

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8811202 [details] [diff] [review]
bug1317678-reroll-stack-probe.patch

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

Hah, that's a nice solution.  Pinging the V8 folks, they like having a clamp on locals, but at the value we have now, 64k, so this seems like the right fix.

::: js/src/wasm/WasmCode.cpp
@@ +388,5 @@
> +    MOZ_ASSERT(offsets.tableEntry - begin_ <= UINT8_MAX);
> +    MOZ_ASSERT(offsets.tableProfilingJump - begin_ <= UINT8_MAX);
> +    MOZ_ASSERT(offsets.nonProfilingEntry - begin_ <= UINT8_MAX);
> +    MOZ_ASSERT(profilingReturn_ - offsets.profilingJump <= UINT8_MAX);
> +    MOZ_ASSERT(profilingReturn_ - offsets.profilingEpilogue <= UINT8_MAX);

I think these, more readable, asserts subsume the 5 equality asserts below, so could you remove those?

Comment 11

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/951fc49fe05e7d3c1e077b92c15367277d8eba43
Bug 1317678 - don't unconditionally unroll stack probe loop. r=luke

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/951fc49fe05e

Comment 13

[areya]

https://hg.mozilla.org/mozilla-central/rev/951fc49fe05e