1230005 - Crash [@ js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator>] or Assertion failure: data >> 28 != 0xf (The instruction does not have condition code), at jit/arm/Assembler-arm.h:1965

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

function af() {
    "use asm";
    function f(d) {
        d = +d
        var i = 0
        i = ~~d
        return i | 0
    }
    return f
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator> (start=start@entry=0xffffb430, dest=dest@entry=0xffffb440, style=style@entry=0xffffb450) at js/src/jit/arm/Assembler-arm.cpp:833
#0  js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator> (start=start@entry=0xffffb430, dest=dest@entry=0xffffb440, style=style@entry=0xffffb450) at js/src/jit/arm/Assembler-arm.cpp:833
#1  0x082d50b4 in js::jit::Assembler::PatchDataWithValueCheck (label=label@entry=..., newValue=..., expectedValue=expectedValue@entry=...) at js/src/jit/arm/Assembler-arm.cpp:3099
#2  0x0810a437 in js::AsmJSModule::staticallyLink (this=0xf7a64000, cx=cx@entry=0xf7a86040) at js/src/asmjs/AsmJSModule.cpp:751
#3  0x0811d506 in js::ValidateAsmJS (cx=0xf7a86040, parser=..., stmtList=stmtList@entry=0xf7a910e8, validated=validated@entry=0xffffb590) at js/src/asmjs/AsmJSValidate.cpp:7000
#4  0x080ba778 in js::frontend::Parser<js::frontend::FullParseHandler>::asmJS (this=0xffffc2b0, list=0xf7a910e8) at js/src/frontend/Parser.cpp:3184
#5  0x080c8bbb in js::frontend::Parser<js::frontend::FullParseHandler>::maybeParseDirective (this=this@entry=0xffffc2b0, list=list@entry=0xf7a910e8, pn=pn@entry=0xf7a91138, cont=cont@entry=0xffffb5f0) at js/src/frontend/Parser.cpp:3258
#6  0x080e6587 in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=this@entry=0xffffc2b0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3324
#7  0x080e8712 in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=this@entry=0xffffc2b0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, kind=kind@entry=js::frontend::Statement, type=type@entry=js::frontend::Parser<js::frontend::FullParseHandler>::StatementListBody) at js/src/frontend/Parser.cpp:1321
#8  0x080e8b8e in js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBodyGeneric (this=this@entry=0xffffc2b0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, pn=pn@entry=0xf7a91038, fun=fun@entry=..., kind=kind@entry=js::frontend::Statement) at js/src/frontend/Parser.cpp:2995
#9  0x080c0824 in js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBody (this=this@entry=0xffffc2b0, inHandling=inHandling@entry=js::frontend::InAllowed, pn=0xf7a91038, fun=fun@entry=..., kind=kind@entry=js::frontend::Statement, generatorKind=generatorKind@entry=js::NotGenerator, inheritedDirectives=..., newDirectives=newDirectives@entry=0xffffb980) at js/src/frontend/Parser.cpp:2798
#10 0x080dcf82 in js::frontend::Parser<js::frontend::FullParseHandler>::functionDef (this=this@entry=0xffffc2b0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, funName=funName@entry=..., kind=kind@entry=js::frontend::Statement, generatorKind=generatorKind@entry=js::NotGenerator, invoked=invoked@entry=js::frontend::Parser<js::frontend::FullParseHandler>::PredictUninvoked) at js/src/frontend/Parser.cpp:2630
#11 0x080dd230 in js::frontend::Parser<js::frontend::FullParseHandler>::functionStmt (this=this@entry=0xffffc2b0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, defaultHandling=defaultHandling@entry=js::frontend::NameRequired) at js/src/frontend/Parser.cpp:3079
#12 0x080e61f0 in js::frontend::Parser<js::frontend::FullParseHandler>::statement (this=this@entry=0xffffc2b0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=true) at js/src/frontend/Parser.cpp:6907
#13 0x080e64f2 in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=this@entry=0xffffc2b0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3302
#14 0x080c4249 in js::frontend::Parser<js::frontend::FullParseHandler>::globalBody (this=0xffffc2b0) at js/src/frontend/Parser.cpp:1051
#15 0x085c8cad in BytecodeCompiler::compileScript (this=this@entry=0xffffbe30, scopeChain=scopeChain@entry=..., evalCaller=evalCaller@entry=...) at js/src/frontend/BytecodeCompiler.cpp:527
#16 0x085c9147 in js::frontend::CompileScript (cx=cx@entry=0xf7a86040, alloc=0xf7a3c18c, scopeChain=scopeChain@entry=..., enclosingStaticScope=enclosingStaticScope@entry=..., evalCaller=evalCaller@entry=..., options=..., srcBuf=..., source_=source_@entry=0x0, extraSct=extraSct@entry=0x0, sourceObjectOut=sourceObjectOut@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:738
#17 0x0836be88 in Compile (cx=cx@entry=0xf7a86040, options=..., scopeOption=HasSyntacticScope, srcBuf=..., script=...) at js/src/jsapi.cpp:4018
#18 0x0836bfc2 in Compile (script=..., length=<optimized out>, chars=<optimized out>, scopeOption=<optimized out>, options=..., cx=0xf7a86040) at js/src/jsapi.cpp:4027
#19 Compile (cx=cx@entry=0xf7a86040, options=..., scopeOption=scopeOption@entry=HasSyntacticScope, bytes=0xf7a1ee00 "function af() {\n    \"use asm\";\n    function f(d) {\n        d = +d\n        var i = 0\n        i = ~~d\n        return i | 0\n    }\n    return f\n}\n", length=142, script=script@entry=...) at js/src/jsapi.cpp:4042
#20 0x0837813f in Compile (script=..., fp=0xf7a86e00, fp@entry=0xf7a8604c, scopeOption=HasSyntacticScope, options=..., cx=0xf7a86040) at js/src/jsapi.cpp:4053
#21 JS::Compile (cx=cx@entry=0xf7a86040, options=..., file=file@entry=0xf7a86e00, script=script@entry=...) at js/src/jsapi.cpp:4093
#22 0x0806ac9c in RunFile (compileOnly=false, file=<optimized out>, filename=0xffffd044 "min.js", cx=0xf7a86040) at js/src/shell/js.cpp:506
#23 Process (cx=cx@entry=0xf7a86040, filename=0xffffd044 "min.js", forceTTY=forceTTY@entry=false, kind=kind@entry=FileScript) at js/src/shell/js.cpp:728
#24 0x08074f1e in ProcessArgs (op=0xffffcd10, cx=<optimized out>) at js/src/shell/js.cpp:6201
#25 Shell (envp=<optimized out>, op=0xffffcd10, cx=<optimized out>) at js/src/shell/js.cpp:6513
#26 main (argc=4, argv=0xffffce54, envp=0xffffce68) at js/src/shell/js.cpp:6874
eax	0x0	0
ebx	0x94693e0	155620320
ecx	0x1000000	16777216
edx	0xf31e0000	-216137728
esi	0xffffb450	-19376
edi	0xf7c89070	-137850768
ebp	0x3100000	51380224
esp	0xffffb3e0	4294947808
eip	0x82de14b <js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator>(js::jit::InstructionIterator*, js::jit::Register*, js::jit::Assembler::RelocStyle*)+251>
=> 0x82de14b <js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator>(js::jit::InstructionIterator*, js::jit::Register*, js::jit::Assembler::RelocStyle*)+251>:	movl   $0x341,0x0
   0x82de155 <js::jit::Assembler::GetPtr32Target<js::jit::InstructionIterator>(js::jit::InstructionIterator*, js::jit::Register*, js::jit::Assembler::RelocStyle*)+261>:	call   0x808ffa0 <abort()>

Comment 1

[Fuzzing Team]

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

Comment 2

[Fuzzing Team]

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

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Looks like a long due error in the ARM jit. When movingWithPatch the symbolic address, we're encoding a PoolHintData but trying to read it as any instruction, when patching the immediate. As a matter of fact, the condition that's read doesn't make any sense.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1230005: Factor out relocation style decision; r=jolesen

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

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r=jolesen

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

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r=luke

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

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

After hours spent understanding how the pools worked, and when the patching occurred and how, plus some dirty details of ARM (e.g. the +8/-8 that appears at different places, thanks Jakob for the explanation), I figured out that we are **never** supposed to see a PoolHintData after the assembler buffer has terminated. Why would this happen with asm.js? The assembler buffer is flushed in CodeGeneratedShared::generateEpilogue, which assertion at the top asserts that it's not running under asm.js. So we're never flushing the assembler buffer. Fix is trivial once this is understood.

Also, this one line change makes it that all asm.js tests pass with --arm-hwcap=vfp (need to add the option flag to nested shells as well for testing caching).

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8701628 [details]
MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r=luke

https://reviewboard.mozilla.org/r/29009/#review25817

Nice job tracking this down!

::: js/src/jit/CodeGenerator.cpp:7920
(Diff revision 1)
> +    // On systems that use a constant pool, this is a good time to emit.

Could you explain a bit more with a comment of the form "Flush constant pools now so that __"?

For my own understanding: I see the assertion is triggered when trying to patch an insn: which one is it and why does this flush fix it?

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8701627 [details]
MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r=jolesen

https://reviewboard.mozilla.org/r/29007/#review25815

Nice. Isn't function call technology just nicer than duplicating code?

::: js/src/jit/arm/Assembler-arm.h:2030
(Diff revision 1)
> +        return (uint32_t*)raw() + offset + 2;

Maybe add a comment here explaining the "+ 2".

Also add an assertion that this is a pc-relative LDR, and not using some other base register.

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8701626 [details]
MozReview Request: Bug 1230005: Factor out relocation style decision; r=jolesen

https://reviewboard.mozilla.org/r/29005/#review25823

Looks good.

::: js/src/jit/arm/MacroAssembler-arm.h:604
(Diff revision 1)
> -        ma_movPatchable(Imm32(imm.value), dest, Always, HasMOVWT() ? L_MOVWT : L_LDR);
> +        ma_movPatchable(Imm32(imm.value), dest, Always);

I wonder if this would also fix https://bugzilla.mozilla.org/show_bug.cgi?id=1221361

::: js/src/jit/arm/MacroAssembler-arm.cpp:430
(Diff revision 1)
> -    switch(rs) {
> +    if (HasMOVWT()) {

This flag can be changed dynamically by the fuzzers. Any chance this function could be made to work under those circumstances?

https://bugzilla.mozilla.org/show_bug.cgi?id=1221361

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

https://reviewboard.mozilla.org/r/29009/#review25817

> Could you explain a bit more with a comment of the form "Flush constant pools now so that __"?
> 
> For my own understanding: I see the assertion is triggered when trying to patch an insn: which one is it and why does this flush fix it?

When using symbolic addresses in Odin, we use movePtr(wasm::SymbolicAddress, Register dest), which ends up calling movPatchable on the ARM backend.
On systems that have MOVW/MOVT, this generates two simple moves, but with the --arm-hwcap=vfp flag at runtime (< armv7), HasMOVWT() returns false, and we have to generate a pool value that is going to contain the address, and a LDR instruction into that pool.

As we have constant pools, the LDR instruction (which always contains a pc-relative offset, in our codebase) needs to be generated in 2 stages:

- we put some bookkeeping data (PoolHintData) in the code stream (that has the same size as the LDR instruction) which encodes all necessary information
- when we flush the constant pool, we patch the actual offset and replace the pool hint data by the actual LDR instruction.

There's an additional stage for AsmJS: during staticallyLink we want to patch the value in the pool (PatchDataWithValueCheck). That ends up checking that we have an actual MOVW/MOVT couple or a LDR instruction, and do the necessary work to patch the value accordingly.

In this bug, as we were never flushing the constant pool, calling PatchDataWithValueCheck would see the pool hint data directly (not the LDR instruction), and this is why the assertion triggers; PoolHintData was designed so as **not** to be confused with an instruction (there's a comment around ExpectedOnes in Assembler-arm.h suggesting that).

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

https://reviewboard.mozilla.org/r/29007/#review25815

> Maybe add a comment here explaining the "+ 2".
> 
> Also add an assertion that this is a pc-relative LDR, and not using some other base register.

So the assertion fails, because sometimes we use other registers. For instance, pushWithPatch calls movPatchable with a scratch register (r12).

There are two ways (at least) to create LDR instructions:
- RetargetFarBranch, which explicitly uses PC and offsets by 8.
- PatchConstantPoolLoad, which ensures the same preconditions as for pc (offset +8 and multiply the index by 4) but with **any** registers.

I wonder whether that means we're clobbering any data in the code stream; I don't think so, otherwise the jit tests would completely fail and they don't. If we were clobbering the code stream, then there would see some crashes at random addresses or random assertEq failures, and there aren't any. So I'll drop the assertion and make a longer comment instead... but there is definitely something bizarre here.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701626 [details]
MozReview Request: Bug 1230005: Factor out relocation style decision; r=jolesen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29005/diff/1-2/

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701627 [details]
MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r=jolesen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29007/diff/1-2/

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701628 [details]
MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r=luke

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29009/diff/1-2/

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701627 [details]
MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r=jolesen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29007/diff/2-3/

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701628 [details]
MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r=luke

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29009/diff/2-3/

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4f5c1196b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bb3765e712
https://hg.mozilla.org/integration/mozilla-inbound/rev/b618547dc136

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c4f5c1196b8
https://hg.mozilla.org/mozilla-central/rev/b9bb3765e712
https://hg.mozilla.org/mozilla-central/rev/b618547dc136

Comment 20

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
Thanks for the explanation!  My remaining question is: if the PatchDataWithValueCheck happens in staticalyLink, after we've masm.finish() (which presumably flushes everything there is to flush), why is the masm.flushBuffer() required *at this point*.  I assume it has something to do with the immediately-following masm.currentOffset() correctly reflecting some offset that depends on pools being flushed, but I don't know what.  That's what I was hoping to have the comment capture so that, at some later point, someone would know that they couldn't move the flushBuffer() to some later point.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #20)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Thanks for the explanation!  My remaining question is: if the
> PatchDataWithValueCheck happens in staticalyLink, after we've masm.finish()
> (which presumably flushes everything there is to flush), why is the
> masm.flushBuffer() required *at this point*.  I assume it has something to
> do with the immediately-following masm.currentOffset() correctly reflecting
> some offset that depends on pools being flushed, but I don't know what. 
> That's what I was hoping to have the comment capture so that, at some later
> point, someone would know that they couldn't move the flushBuffer() to some
> later point.

Makes sense. My new (tentative) explanation is that the masm used during compilation might not be the same as the one in ModuleGenerator::finish, because of parallel compilation, and the constant pool bookkeeping information gets lost when merging the masms.

Funnily, with the latest changes to asm.js, if we remove the flushBuffer call in CodeGenerator, we have another assertion triggering in ModuleGenerator::finishTask, complaining that there's a pool which hasn't been flushed. Would have been more helpful a few days ago :-)

Transferring the constant pool information upon merging seems overkill (basically, we should implement something like IonAssemblerBufferWithConstantPool::mergeWith). The only win would be to more optimally fill the constant pools, I imagine? It seems like a high complexity (risk) / low reward situation, though. Using flushBuffer seems simpler and better.

We could probably move the flushBuffer call to ModuleGenerator::finishTask; just before merging the masms, we'd flushBuffer the results.masm(). That sounds fine as well (note that in CodeGenerator::generateAsmJS, the next thing after flushBuffer was recording the offsets->end offset, but PC shouldn't ever be in the constant pool directly anyways, so it seems fine to delay the call to flushBuffer).

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: move.patch

See previous comment. This delays the flushBuffer call to WasmGenerator::finishTask, which is the last minute (calling results.masm().size() would otherwise complain that the constant pool isn't empty / hasn't been flushed).

Comment 23

[Luke Wagner [:luke]]

Ah hah!  So merging is the reason!  That makes sense; glad to track this culprit down.  In that case, though, how about moving this into asmMergeWith() instead?  It seems like an impl detail of that operation.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #23)
> Ah hah!  So merging is the reason!  That makes sense; glad to track this
> culprit down.  In that case, though, how about moving this into
> asmMergeWith() instead?  It seems like an impl detail of that operation.

I would have done that (we're already flushing the whole-masm in AssemblerARM::asmMergeWith), but that discards the const qualifier of the other-masm (to be merged) in asmMergeWith.

Comment 25

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> I would have done that (we're already flushing the whole-masm in
> AssemblerARM::asmMergeWith), but that discards the const qualifier of the
> other-masm (to be merged) in asmMergeWith.

Hrm, how about a const_cast?

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: flush.patch

Removed the const keyword in asmMergeWith, MacroAssembler and ARM only.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8702643 [details] [diff] [review]
flush.patch

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

Perfect, thanks!

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/158232cba907

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/158232cba907