Closed Bug 888578 Opened 11 years ago Closed 11 years ago

Assertion failure: isFloatReg(), at ion/MoveResolver.h

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: gkw, Assigned: jld)

References

Details

(Keywords: assertion, regression)

Attachments

(3 files, 4 obsolete files)

possible stack from the log
3.73 KB, text/plain
Details
gdb-backtrace.txt
6.52 KB, text/plain
Details
Extend ARM masm's floatArgsInGPR workaround to memory loads (with more comments)
4.71 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review 
The Android debug tests are blocked by this bug:

Assertion failure: isFloatReg(), at ion/MoveResolver.h

See:

https://tbpl.mozilla.org/php/getParsedLog.php?id=24717830&tree=Cedar#error10


All logs located at:

https://tbpl.mozilla.org/?tree=Cedar&showall=1&jobname=android.*debug

Setting needinfo from Marty, please feel free to forward this on if incorrect.
Flags: needinfo?(mrosenberg)
I hit this assertion failure when running any Robocop test on mozilla-central.
Keywords: regression
Dan, you landed some move resolver patches recently, could this be a regression from one of your patches?
Flags: needinfo?(sunfish)
In theory my patches were for x86/x64 and shouldn't have affected ARM, but they did touch some shared code so it is possible.

I'm not currently set up to do this level of debugging on ARM. I will pursue it. In the meantime, if anyone could post a stack trace at the point of the assertion failure, it may help show us where to look.
Flags: needinfo?(sunfish)
Does the possible stack in comment 1 help?
Attached file gdb-backtrace.txt — 
Dan, I captured this stack trace in gdb (with an optimized debug build, so the stack might be a little wonky).

JS_ASSERT(isFloatReg()) fails because the MoveOperand's kind_ == FLOAT_ADDRESS, not FLOAT_REG:

(gdb) print *this
$1 = {kind_ = js::ion::MoveResolver::MoveOperand::FLOAT_ADDRESS, code_ = 5, disp_ = 0}
Flags: needinfo?(sunfish)
Thanks. It looks like it's coming from the floatReg() call in MacroAssemblerARMCompat::passABIArg, which is called like this:

        if (from.isDouble()) {
            floatArgsInGPR[destReg.code() >> 1] = VFPRegister(from.floatReg());

As Chris points out, isDouble() checks for FLOAT_REG or FLOAT_ADDRESS, while floatReg() requires FLOAT_REG. It appears this code predates FLOAT_ADDRESS, which was introduced in 680d346756b7.

Shu-yu Guo, could you take a look?
Flags: needinfo?(sunfish) → needinfo?(shu)
Attached patch fix (obsolete) — — Splinter Review 
sunfish, thanks for tracking this down.

Could you try if this patch works for you? I think the use of |isDouble()| in the HARDFP version works as intended -- it adds a move resolver move iff the from/to operands aren't already both float regs and aren't already in the same register, so that just leaves the SOFTP version being incorrect and another assert in the ARM move emitter.
Attachment #769745 - Flags: review?(mrosenberg)
Flags: needinfo?(shu)
I was hitting this on a local debug build (target = Nexus 10); I added this patch but still fail.  Failure is on startup, in thread 2, but the backtrace is garbage.
Clobbered just in case; same failure
We're hitting this on FxOS phones building against m-c too.
After applying patch, FxOS m-c still fails. Message:

F/MOZ_Assert( 5254): Assertion failure: from.isMemory(), at /share/mozbuild/B2G/gecko/js/src/ion/arm/MoveEmitter-arm.cpp:227
F/libc    ( 5254): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
I just cobbered my mozilla-central build (cset 4ffb23062b3b without Shu's patch) and the startup assertion failure "went away", but after I applied some patches and did an incremental rebuild, I hit the same assertion.

With Shu's patch, I hit a different assertion failure:

F/MOZ_Assert(16141): Assertion failure: from.isMemory(), at /Users/cpeterson/Code/mozilla/central/js/src/ion/arm/MoveEmitter-arm.cpp:227

I am running on a Nexus 4.
I just checked; my failure after patch-and-clobber is the same as cpetersons:

F/MOZ_Assert(22881): Assertion failure: from.isMemory(), at ../../../js/src/ion/arm/MoveEmitter-arm.cpp:227
F/libc    (22881): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 23088 (Gecko)

Sorry, I missed that the actual message changed
If Dan Gohman's patches are the cause, perhaps they could be backed out until this is resolved?  Which ones are they? Bug number?
Comment on attachment 769745 [details] [diff] [review]
fix

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

This doesn't look wrong.  As long as it makes the problem go away, r+.
Attachment #769745 - Flags: review?(mrosenberg) → review+
Attached patch fix v2 (obsolete) — — Splinter Review 
Fix Chris's crash as well, carrying r+ as change is trivial.
Assignee: general → shu
Attachment #769745 - Attachment is obsolete: true
Attachment #769884 - Flags: review+
Judging from various comments this bug seems fairly urgent. However, I'm currently in Europe and will be in transit tomorrow and be generally available. 

Would someone be so kind as to check the updated patch if try looks green?
I am wondering though, this bug has been there for a while... why is it showing up now? I'm not aware of non-parallel code that generates passing a double ABI/VM argument in Ion.
(In reply to Shu-yu Guo [:shu] from comment #20)
> Judging from various comments this bug seems fairly urgent. However, I'm
> currently in Europe and will be in transit tomorrow and be generally
> available. 
> 
> Would someone be so kind as to check the updated patch if try looks green?

Wow I typoed several words there.

I will generally be *unavailable* tomorrow, so I'd like to check the updated patch *in* if try looks green.
(In reply to Shu-yu Guo [:shu] from comment #18)
> Created attachment 769884 [details] [diff] [review]
> fix v2

With this patch, I hit a new assertion failure:

F/MOZ_Assert(26624): Assertion failure: Tag == MEM, at /Users/cpeterson/Code/mozilla/central/js/src/ion/arm/Assembler-arm.h:1102
Do you have a stack for that assert?
F/MOZ_Assert(14144): Assertion failure: Tag == MEM, at ../../../js/src/ion/arm/Assembler-arm.h:1102
F/libc    (14144): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 14169 (Gecko)

Thread 2

0x817458e8 in ?? ()
(gdb) where
#0  0x817458e8 in ?? ()
#1  0x81743604 in ?? ()
#2  0x81743604 in ?? ()
Here's a working stacktrace:

0x7dbedc4a in disp (this=<optimized out>)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/Assembler-arm.h:1102
1102	        JS_ASSERT(Tag == MEM);
(gdb) bt
#0  0x7dbedc4a in disp (this=<optimized out>)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/Assembler-arm.h:1102
#1  js::ion::MacroAssemblerARM::ma_vdtr (this=0x753aad40, ls=js::ion::IsStore, addr=..., rt=..., 
    cc=js::ion::Assembler::Always)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MacroAssembler-arm.cpp:1387
#2  0x7dbee11e in js::ion::MacroAssemblerARM::ma_vstr (this=<optimized out>, src=..., addr=..., 
    cc=<optimized out>) at /home/morbo/hg/mozilla-central/js/src/ion/arm/MacroAssembler-arm.cpp:1452
#3  0x7dbf51b0 in js::ion::MoveEmitterARM::emitDoubleMove (this=0x753aaca0, from=..., to=...)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MoveEmitter-arm.cpp:230
#4  0x7dbf5238 in js::ion::MoveEmitterARM::emit (this=0x753aaca0, move=...)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MoveEmitter-arm.cpp:252
#5  0x7dbf52b6 in js::ion::MoveEmitterARM::emit (this=0x753aaca0, moves=...)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MoveEmitter-arm.cpp:33
#6  0x7dbf24fa in js::ion::MacroAssemblerARMCompat::callWithABIPre (this=0x753aad40, 
    stackAdjust=<optimized out>)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MacroAssembler-arm.cpp:3149
#7  0x7dbf2694 in js::ion::MacroAssemblerARMCompat::callWithABI (this=0x753aad40, fun=
    0x7db9645d <js::ion::ParDoubleToString(js::ForkJoinSlice*, double, JS::MutableHandle<JSString*>)>, 
    result=js::ion::MacroAssemblerARMCompat::GENERAL)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/MacroAssembler-arm.cpp:3194
#8  0x7dad200a in js::ion::MacroAssembler::callWithABI<void*> (this=0x753aad40, 
    fun=@0x7eb9d424: 0x7db9645d <js::ion::ParDoubleToString(js::ForkJoinSlice*, double, JS::MutableHandle<JSString*>)>, result=js::ion::MacroAssemblerARMCompat::GENERAL)
    at /home/morbo/hg/mozilla-central/js/src/ion/IonMacroAssembler.h:702
#9  0x7dbf8f52 in js::ion::IonRuntime::generateVMWrapper (this=0x8492f2e0, cx=0x7f079040, f=...)
    at /home/morbo/hg/mozilla-central/js/src/ion/arm/Trampoline-arm.cpp:684
#10 0x7daf13d0 in js::ion::IonRuntime::initialize (this=0x8492f2e0, cx=0x7f079040)
    at /home/morbo/hg/mozilla-central/js/src/ion/Ion.cpp:264
#11 0x7d92fa56 in JSRuntime::createIonRuntime (this=0x7f01c000, cx=0x7f079040)
    at /home/morbo/hg/mozilla-central/js/src/jscompartment.cpp:122
#12 0x7d92fad6 in getIonRuntime (cx=0x7f079040, this=<optimized out>)
    at /home/morbo/hg/mozilla-central/js/src/jscntxt.h:810
#13 JSCompartment::ensureIonCompartmentExists (this=0x848ef800, cx=0x7f079040)
    at /home/morbo/hg/mozilla-central/js/src/jscompartment.cpp:144
#14 0x7dab354c in CanEnterBaselineJIT (cx=0x7f079040, script=..., osr=<optimized out>)
    at /home/morbo/hg/mozilla-central/js/src/ion/BaselineJIT.cpp:238
#15 0x7dab3f9c in js::ion::CanEnterBaselineMethod (cx=0x7f079040, state=...)
    at /home/morbo/hg/mozilla-central/js/src/ion/BaselineJIT.cpp:324
#16 0x7d84c2ec in js::RunScript (cx=0x7f079040, state=...)
    at /home/morbo/hg/mozilla-central/js/src/vm/Interpreter.cpp:421
#17 0x7d84c612 in js::ExecuteKernel (cx=0x7f079040, script=..., scopeChainArg=..., thisv=..., 
    type=js::EXECUTE_GLOBAL, evalInFrame=..., result=0x753ac6a8)
    at /home/morbo/hg/mozilla-central/js/src/vm/Interpreter.cpp:620
#18 0x7d84cb32 in js::Execute (cx=0x7f079040, script=..., scopeChainArg=..., rval=0x753ac6a8)
    at /home/morbo/hg/mozilla-central/js/src/vm/Interpreter.cpp:657
#19 0x7d8f06b2 in JS_ExecuteScript (cx=0x7f079040, objArg=<optimized out>, scriptArg=0x84a2b120, 
    rval=0x753ac6a8) at /home/morbo/hg/mozilla-central/js/src/jsapi.cpp:5465
#20 0x7c3daf8c in nsJSContext::ExecuteScript (this=0x8491e470, aScriptObject_=0x84a2b120, 
    aScopeObject_=0x84a1e040) at /home/morbo/hg/mozilla-central/dom/base/nsJSEnvironment.cpp:1412
#21 0x7c34eb02 in mozilla::dom::XULDocument::ExecuteScript (this=0x821dc800, aContext=0x8491e470,
I have what seems to be a fix — it unblackscreens a b2g debug build.
Assignee: shu → jld
Flags: needinfo?(mrosenberg)
This patch unblackscreens b2g for me, but I suspect it's wrong, because:

If we're calling a function at (double, int), and the first parameter lives in memory, and the source for the second parameter happens to be r1, then — if I understand this part of the assembler correctly, and I may not — there's nothing to prevent us from loading the double into r0,r1 before we try to move the int from r1 into r2, because the value-shuffling doesn't know about register pairs and thinks it's just r0.

And that's where :mjrosenb's floatArgsInGPR thing comes in — by moving the vxfer(s) to the end, the only conflict that they could participate in is if their source is clobbered by a move, *but*: it's used only for the softfp ABI, so there are no float argument registers (ScratchFloatReg can be clobbered, but I'm assuming it can't be the source of an argument).
I am seeing the same crash cpeterson reported in comment 7, using a debug build from m-c with some unrelated local changes. The crash happens reliably on startup. Applying the patch from comment 28 seems to make it go away.
Fixes the b2g blackscreen, and in theory should be doing things correctly, but I haven't run tests yet.

Here's a disassembled VMFunction wrapper for ParDoubleToString: http://pastebin.mozilla.org/2586597 (note the ldrd instruction)
Attachment #769884 - Attachment is obsolete: true
Attachment #770293 - Attachment is obsolete: true
Attachment #770383 - Flags: review?(mrosenberg)
For reference, it's not just a parallel-JS thing; the regular DoubleToString needs an ldrd as well: http://pastebin.mozilla.org/2586680

The reason the parallel one showed up in comment #26 is, I assume, just because its wrapper was generated first.
Comment on attachment 770383 [details] [diff] [review]
Extend ARM masm's floatArgsInGPR workaround to memory loads

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

It all seems good, but I'd like to see a try run that shows we haven't introduced any new bugs.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +3160,5 @@
> +            if (from.isFloatReg()) {
> +                ma_vxfer(VFPRegister(from.floatReg()), to0, to1);
> +            } else {
> +                JS_ASSERT(from.isFloatAddress());
> +                ma_ldrd(EDtrAddr(from.base(), EDtrOffImm(from.disp())), to0, to1);

this is a bit non-future proof.  EDtrOffImm can only hold an offset of 255.  I'm told this value currently is never above 0, but we should at least add an assert for the time being.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +419,5 @@
>  #endif
>      bool dynamicAlignment_;
>  
>      bool enoughMemory_;
> +    MoveResolver::MoveOperand floatArgsInGPR[2];

Seeing a MoveOperand outside of the move resolver/move emitter makes the hairs on the back of my neck stand up, but this seems to be the sanest way of getting the data where it needs to be.
Attachment #770383 - Flags: review?(mrosenberg) → review+
(In reply to Jed Davis [:jld] from comment #31)
> For reference, it's not just a parallel-JS thing; the regular DoubleToString
> needs an ldrd as well: http://pastebin.mozilla.org/2586680
> 
> The reason the parallel one showed up in comment #26 is, I assume, just
> because its wrapper was generated first.

That's right, both are VM calls with double for the second argument.
try should finally be working now, so if I've done everything right then eventually results should appear here: https://tbpl.mozilla.org/?tree=Try&rev=5d7a33af814c
Let's try that again with less of me failing at trychooser, and use debug builds, which are the problem here: https://tbpl.mozilla.org/?tree=Try&rev=4be16a0f548d
But, manage your expectations, we only actually do anything which involves running debug builds on Cedar, not on any other tree, so all you can tell on try is whether or not it compiles (cross-compiles, on a Linux slave), not whether or not it will run, because there are no debug tests so -u all is exactly the same as -u none.
Oh.  I didn't realize that try worked like that, though I guess in hindsight it's obvious.  So, let us return our attention to the try URL in comment #34, where I restarted the builds I'd conscientiously killed hours before when I thought I wouldn't need them.

Things seem to be passing so far.

Meanwhile, on a b2g debug build, I tried running jit-tests.  With my change, 34 of them fail, which is about what I'd expect (the last time I tried it was 33).  Without my change, they *all* fail.  As does `jsshell -e 0`.
I can confirm that the latest patch fixes my startup crash.
(In reply to Marty Rosenberg [:mjrosenb] from comment #32)
> > +                ma_ldrd(EDtrAddr(from.base(), EDtrOffImm(from.disp())), to0, to1);
> 
> this is a bit non-future proof.  EDtrOffImm can only hold an offset of 255. 
> I'm told this value currently is never above 0, but we should at least add
> an assert for the time being.

Nice catch.  But I see that EDtrOffImm's constructor already asserts that the value is within [-255,255], so I think we're covered on that front.  I'll add a comment so at least readers know we're taking a shortcut.

For reference, is r12 available at this point if we need a temporary?  (Not that I particularly want to add that case without a way to test it.)

> Seeing a MoveOperand outside of the move resolver/move emitter makes the
> hairs on the back of my neck stand up, but this seems to be the sanest way
> of getting the data where it needs to be.

I'll add more comments: http://pastebin.mozilla.org/2587914
Oranges on the try run seem to be bug 711189 and bug 817339 — known intermittent issues.
Attachment #770674 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/fbd476579542
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 890271
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: