Closed
Bug 1230005
Opened 9 years ago
Closed 8 years ago
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
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: decoder, Assigned: bbouvier)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(4 files, 1 obsolete file)
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()>
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Comment 1•9 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Updated•9 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Updated•9 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Updated•9 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:]
Comment 2•9 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Assignee | ||
Comment 3•9 years ago
|
||
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.
Flags: needinfo?(benj)
Updated•9 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/29005/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29005/
Attachment #8701626 -
Flags: review?(jolesen)
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/29007/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29007/
Attachment #8701627 -
Flags: review?(jolesen)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/29009/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29009/
Attachment #8701628 -
Flags: review?(luke)
Assignee | ||
Comment 7•8 years ago
|
||
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).
Flags: needinfo?(benj)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → benj
Status: NEW → ASSIGNED
Updated•8 years ago
|
Attachment #8701628 -
Flags: review?(luke) → review+
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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.
Attachment #8701627 -
Flags: review?(jolesen) → review+
Comment 10•8 years ago
|
||
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
Attachment #8701626 -
Flags: review?(jolesen) → review+
Assignee | ||
Comment 11•8 years ago
|
||
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).
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
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/
Attachment #8701626 -
Attachment description: MozReview Request: Bug 1230005: Factor out relocation style decision; r?jolesen → MozReview Request: Bug 1230005: Factor out relocation style decision; r=jolesen
Assignee | ||
Comment 14•8 years ago
|
||
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/
Attachment #8701627 -
Attachment description: MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r?jolesen → MozReview Request: Bug 1230005: Hide specifics of the LDR instruction; r=jolesen
Assignee | ||
Comment 15•8 years ago
|
||
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/
Attachment #8701628 -
Attachment description: MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r?luke → MozReview Request: Bug 1230005: Flush the assembler buffer at the end of asm.js compilation; r=luke
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9c4f5c1196b8 https://hg.mozilla.org/mozilla-central/rev/b9bb3765e712 https://hg.mozilla.org/mozilla-central/rev/b618547dc136
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 20•8 years ago
|
||
(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.
Assignee | ||
Comment 21•8 years ago
|
||
(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).
Assignee | ||
Comment 22•8 years ago
|
||
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).
Attachment #8702530 -
Flags: review?(luke)
Comment 23•8 years ago
|
||
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.
Assignee | ||
Comment 24•8 years ago
|
||
(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•8 years ago
|
||
(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?
Assignee | ||
Comment 26•8 years ago
|
||
Removed the const keyword in asmMergeWith, MacroAssembler and ARM only.
Attachment #8702530 -
Attachment is obsolete: true
Attachment #8702530 -
Flags: review?(luke)
Attachment #8702643 -
Flags: review?(luke)
Comment 27•8 years ago
|
||
Comment on attachment 8702643 [details] [diff] [review] flush.patch Review of attachment 8702643 [details] [diff] [review]: ----------------------------------------------------------------- Perfect, thanks!
Attachment #8702643 -
Flags: review?(luke) → review+
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/158232cba907
You need to log in
before you can comment on or make changes to this bug.
Description
•