1202757 - ARM disassembler does not understand exclusive instructions and barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Consider:

[Codegen] instruction AsmJSAtomicBinopHeapForEffect
[Codegen]    b45d50e4  e08be000       add lr, fp, r0
[Codegen]    b45d50e8  f57ff05f       unknown
[Codegen]                         1004:
[Codegen]    b45d50ec  e1decf9f       unknown
[Codegen]    b45d50f0  e08cc001       add ip, ip, r1
[Codegen]    b45d50f4  e1cecf9c       unknown
[Codegen]    b45d50f8  e35c0001       cmp ip, #1
[Codegen]    b45d50fc  0afffffa       beq  -> 1004
[Codegen]    b45d5100  f57ff05f       unknown

The four unknowns here should have been DMB, LDREXB, STREXB, and DMB.

(Disassembler came in via bug 1157934.)

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1202757-disasm-more.patch

Handle ldrex, strex, dmb, and dsb.  Example output from a -D run:

[AtomicTypedArrayElementBinop]
   026f625c  f57ff05f       dmb sy
                        1002:
   026f6260  e1903f9f       ldrex r3, [r0]
   026f6264  e083c001       add ip, r3, r1
   026f6268  e1802f9c       strex r2, ip, [r0]
   026f626c  e3520001       cmp r2, #1
   026f6270  0afffffa       beq  -> 1002
   026f6274  f57ff05f       dmb sy

Memo to self & others: whenever a new instruction is added to the simulator we should now check the disassembler too.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 8659200 [details] [diff] [review]
bug1202757-disasm-more.patch

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

::: js/src/jit/arm/disasm/Disasm-arm.cpp
@@ +801,5 @@
> +                // The documentation names the low four bits of the
> +                // store-exclusive instructions "Rt" but canonically
> +                // they are really "Rm".
> +                switch (instr->Bits(27, 20)) {
> +                  case 0x18:

The ARM code currently, regrettably, uses hardcoded instruction constants all over the place. If you would like to name this constant and share it with as_strex() [0x01800f90], and so on for each, that would be an improvement to the situation. If you do not feel like doing that, then at least it will not have gotten much worse.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/839bd50e5950

Comment 4

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #2)
> Comment on attachment 8659200 [details] [diff] [review]
> bug1202757-disasm-more.patch
> 
> The ARM code currently, regrettably, uses hardcoded instruction constants
> all over the place. If you would like to name this constant and share it
> with as_strex() [0x01800f90], and so on for each, that would be an
> improvement to the situation. If you do not feel like doing that, then at
> least it will not have gotten much worse.

I did part of this: I named appropriate constants and shared them between the disassembler and the simulator, where the sharing was natural.  (Every little bit helps, hopefully.)

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/839bd50e5950