1438827 - Assertion failures while tracing ARM assembler: data >> 28 != 0xf

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: test-trace-jitcode

I added code to CodeGenerator::link() to trace the macro assembler and this provoked many assertion failures on ARM builds of the form:

Assertion failure: data >> 28 != 0xf (The instruction does not have condition code), at /home/jon/clone/dev/js/src/jit/arm/Assembler-arm.h:2011

Patch attached that triggers this.

I couldn't work out a way to trigger this in an unmodified shell, but the macro assembler trace method is certainly called so this should be possible.

I don't think this is high risk, but marking s-s due to it being a GC tracing issue.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: arm-assembler-changes

There are two problems here.

Firstly, ma_mov(ImmGCPtr) writes the data relocation offset based on nextOffset().  That may end up pointing to a pool guard if we have to insert one.  The patch changes ma_movPatchable() to return the offset the first instruction was written to.

Secondly, BufferInstructionIterator::maybeSkipAutomaticInstructions() doesn't correctly skip over constant pools.  For some reason the implementation is different to that of InstructionIterator which is really confusing.  The latter version does |advanceRaw(1 + ph->size())| to skip over the pool plus guard instruction, where as the former calls next() which just advances by the instruction size.  So I think we need to also advance by the pool size too.

With these changes jit-tests pass on ARM simulator builds, with a single failure that is unrelated to this patch and can be addressed separately.

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8951679 [details] [diff] [review]
arm-assembler-changes

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

I believe this.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +3085,1 @@
>          return next();

If it were me I would add a comment before the new line here that says something to the effect that "together the next two statements skip the current instruction and the subsequent header", since the first one skips the current instruction and part of the header and the second one skips the rest of the header.   Maybe that is overkill; I tend to like comments.

Comment 3

[Jon Coppeard (:jonco)]

I dug a bit and found bug 1393011 comment 11.  Sean, is that comment right?  I couldn't find where AssemblerBufferInstIterator::advance() skips pools.

https://searchfox.org/mozilla-central/source/js/src/jit/shared/IonAssemblerBuffer.h#422

Comment 4

[Sean Stangl [:sstangl]]

Jon: "BufferOffsets" automatically skip over pools. They are a way of viewing the underlying buffer linearly, such that no matter where pools are, `(buf+1)` is always the "next user-placed instruction" after `(buf)`.

The logic that skips over pools is implemented at lookup time, when you use a BufferOffset to index into the underlying buffer, in `getInst()`, here: https://searchfox.org/mozilla-central/source/js/src/jit/shared/IonAssemblerBuffer.h#376

It uses a "finger" to remember the Slice that contained the last BufferOffset that was looked-up, so that it's usually quite fast and not O(n).

Comment 5

[Jon Coppeard (:jonco)]

Following further discussion on IRC, Sean OKed this patch.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ec9a7399faf4dd1e18bb43ba04d456a2ed23d0

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f9ec9a7399fa