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

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla46
ARM
Linux
assertion, crash, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 1

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

Updated

3 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Whiteboard: [jsbugmon:] → [jsbugmon:update]

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:]

Comment 2

3 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
(Assignee)

Comment 3

3 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)
Whiteboard: [jsbugmon:] → [jsbugmon:update]
(Assignee)

Comment 4

3 years ago
Created attachment 8701626 [details]
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/
Attachment #8701626 - Flags: review?(jolesen)
(Assignee)

Comment 5

3 years ago
Created attachment 8701627 [details]
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/
Attachment #8701627 - Flags: review?(jolesen)
(Assignee)

Comment 6

3 years ago
Created attachment 8701628 [details]
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/
Attachment #8701628 - Flags: review?(luke)
(Assignee)

Comment 7

3 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

3 years ago
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8701628 - Flags: review?(luke) → review+
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 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+
See Also: → bug 1221361
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

3 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

3 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

3 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

3 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

3 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

3 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

3 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 19

3 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
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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

3 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

3 years ago
Created attachment 8702530 [details] [diff] [review]
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).
Attachment #8702530 - Flags: review?(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.
(Assignee)

Comment 24

3 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.
(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

3 years ago
Created attachment 8702643 [details] [diff] [review]
flush.patch

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 on attachment 8702643 [details] [diff] [review]
flush.patch

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

Perfect, thanks!
Attachment #8702643 - Flags: review?(luke) → review+
You need to log in before you can comment on or make changes to this bug.