1252326 - Assertion failure: pool_.numEntries() == 0, at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:711 with ASM.js and --arm-hwcap=vfp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 5e0140b6d118 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-simulator=arm --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --arm-hwcap=vfp):

a = function(global, env, buffer) {
  'use asm'
  var HEAP32 = new global.Int32Array(buffer);
  var HEAPF32 = new global.Float32Array(buffer);
  function x(i1, i2, d3, i4) {
  i1 = i1 | 0;
  i2 = i2 | 0;
  d3 = +d3;
  i4 = i4 | 0;
  var i19 = 0;
  i19 = ~~HEAPF32[0];
  HEAP32[0] = i19;
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x08093f7e in js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::size (this=0xf7a56254) at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:711
#0  0x08093f7e in js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::size (this=0xf7a56254) at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:711
#1  0x08451cbf in size (this=0xf7a56254) at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:711
#2  js::jit::Assembler::size (this=this@entry=0xf7a5603c) at js/src/jit/arm/Assembler-arm.cpp:1332
#3  0x081d55cc in js::wasm::ModuleGenerator::finishTask (this=this@entry=0xffff9f08, task=0xf7a56000) at js/src/asmjs/WasmGenerator.cpp:310
#4  0x081dddda in js::wasm::ModuleGenerator::finishFuncDef (this=0xffff9f08, funcIndex=0, generateTime=0, fg=fg@entry=0xffff9848) at js/src/asmjs/WasmGenerator.cpp:826
#5  0x081e6ed4 in finish (generateTime=<optimized out>, funcIndex=<optimized out>, this=0xffff9840) at js/src/asmjs/AsmJS.cpp:2638
#6  CheckFunction (m=...) at js/src/asmjs/AsmJS.cpp:6547
#7  0x081ee483 in CheckFunctions (m=...) at js/src/asmjs/AsmJS.cpp:6578
#8  CheckModule (cx=cx@entry=0xf7a76020, parser=..., stmtList=stmtList@entry=0xf7a83638, moduleObj=moduleObj@entry=..., time=time@entry=0xffffa8b0, slowFuncs=slowFuncs@entry=0xffffa900) at js/src/asmjs/AsmJS.cpp:6789
#9  0x081ef3dc in js::CompileAsmJS (cx=0xf7a76020, parser=..., stmtList=stmtList@entry=0xf7a83638, validated=validated@entry=0xffffaa40) at js/src/asmjs/AsmJS.cpp:8066
#10 0x0812a0a5 in js::frontend::Parser<js::frontend::FullParseHandler>::asmJS (this=0xffffbe30, list=0xf7a83638) at js/src/frontend/Parser.cpp:3413
[...]
#41 main (argc=5, argv=0xffffcb84, envp=0xffffcb9c) at js/src/shell/js.cpp:7244
eax	0x0	0
ebx	0x984bb94	159693716
ecx	0xf7e3a88c	-136075124
edx	0x0	0
esi	0xf7a56000	-140156928
edi	0x0	0
ebp	0xffff95f8	4294940152
esp	0xffff95e0	4294940128
eip	0x8093f7e <js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::size() const+42>
=> 0x8093f7e <js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::size() const+42>:	movl   $0x2c7,0x0
   0x8093f88 <js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::size() const+52>:	call   0x80fb6e0 <abort()>

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Can't call masm.size() before calling masm.flush() on this flavor of ARM. In WasmGenerator::finishIonTask, there's a call to flush() in asmMergeWith(), but this is happening way after the first call to results.masm().size(), which causes the issue here.

Luke, where's the best place to put the flush() call on results.masm()?

- we'd like to avoid platform specific code in WasmGenerator::finishTask, so not there.
- for the same reason, not in CodeGenerator::generateAsmJS.
- CodeGenerator::generateOutOfLineCode is the last function generating code in generateAsmJS, so it looks like a good candidate. But it's also used for non ion code generation, which also generates code *after* the call to generateOutOfLineCode.

I'm wondering whether we want a special "size" function, maybe called, ensureFlushedSize(), that would
- just return size() on non-ARM platforms
- ensure flush() and then return size() on ARM

What do you think?

Comment 2

[Luke Wagner [:luke]]

Thanks for looking into this!  How about we add a nop flush() for !ARM and then put a flush() call in CodeGenerator::generateAsmJS() after generateOutOfLineCode() (in case anything ever gets added after generateOutOfLineCode()) and before the "offsets->end = masm.currentOffset()"?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1252326: Flush the assembly buffer at the end of codegen for plateforms which need it; r?luke

Review commit: https://reviewboard.mozilla.org/r/37575/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37575/

Comment 4

[Nicolas B. Pierron [:nbp]]

https://reviewboard.mozilla.org/r/37575/#review34105

::: js/src/jit/MacroAssembler.h:525
(Diff revision 1)
> +    void flush() PER_SHARED_ARCH;

The Macro Assembler uses sections dedicated to one topic, in both the headers, and the implementations files.

Make sure this "flush" function is in the correct section, i-e not the one dedicated to ABI function calls.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8725600 [details]
MozReview Request: Bug 1252326: Flush the assembly buffer at the end of codegen for plateforms which need it; r?luke

https://reviewboard.mozilla.org/r/37575/#review34133

Comment 6

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f52b56a1b6c

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Reorder MacroAssembler flush functions to follow the header order.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2213f8796d4f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f52b56a1b6c
https://hg.mozilla.org/mozilla-central/rev/2213f8796d4f