This is an archive of the discontinued LLVM Phabricator instance.

[ARMInstPrinter] Print the target address of a branch instruction
ClosedPublic

Authored by ikudrin on Jun 22 2021, 4:34 AM.

Details

Summary

This follows other patches that changed printing immediate values of branch instructions to target addresses, see D76580 (x86), D76591 (PPC), D77853 (AArch64).

As observing immediate values might sometimes be useful, they are printed as comments for branch instructions.

// llvm-objdump -d output (before)
000200b4 <_start>:
   200b4: ff ff ff fa   blx     #-4 <thumb>
000200b8 <thumb>:
   200b8: ff f7 fc ef   blx     #-8 <_start>

// llvm-objdump -d output (after)
000200b4 <_start>:
   200b4: ff ff ff fa   blx     0x200b8 <thumb>         @ imm = #-4
000200b8 <thumb>:
   200b8: ff f7 fc ef   blx     0x200b4 <_start>        @ imm = #-8

// GNU objdump -d.
000200b4 <_start>:
   200b4:       faffffff        blx     200b8 <thumb>
000200b8 <thumb>:
   200b8:       f7ff effc       blx     200b4 <_start>

Diff Detail

Event Timeline

ikudrin created this revision.Jun 22 2021, 4:34 AM

@peter.smith, the patch helped me to spot a couple of suspicious places:

  • In lld/test/ELF/arm-fix-cortex-a8-blx.s, the target address in the patch points to <_start+0x4> while the initial code targets <_start>;
  • In lld/test/ELF/arm-thumb-interwork-thunk.s, there are check blocks that are not validated, CHECK-PI-ARM-PLT and CHECK-PI-THUMB-PLT. It looks like they were not checked from the very beginning.

Could you possibly take a look?

@peter.smith, the patch helped me to spot a couple of suspicious places:

  • In lld/test/ELF/arm-fix-cortex-a8-blx.s, the target address in the patch points to <_start+0x4> while the initial code targets <_start>;
  • In lld/test/ELF/arm-thumb-interwork-thunk.s, there are check blocks that are not validated, CHECK-PI-ARM-PLT and CHECK-PI-THUMB-PLT. It looks like they were not checked from the very beginning.

Could you possibly take a look?

I can give it a try. Now on my todo list. I'll have to do this in my spare time so it may take a few days.

@peter.smith, the patch helped me to spot a couple of suspicious places:

  • In lld/test/ELF/arm-fix-cortex-a8-blx.s, the target address in the patch points to <_start+0x4> while the initial code targets <_start>;
  • In lld/test/ELF/arm-thumb-interwork-thunk.s, there are check blocks that are not validated, CHECK-PI-ARM-PLT and CHECK-PI-THUMB-PLT. It looks like they were not checked from the very beginning.

Could you possibly take a look?

I can give it a try. Now on my todo list. I'll have to do this in my spare time so it may take a few days.

There's a couple of things wrong with the unrelocated BLX case in the LLD code (test lld/test/ELF/arm-fix-cortex-a8-blx.s). To the best of my knowledge nothing generates unrelocated BLX instructions so this shouldn't affect any real world program. However it still should be fixed. I'll try and do that in the next couple of days.

  • In getThumbDestAddr when the instruction is a BLX we should set sourceAddr = alignDown(sourceAddr, 4);
  • in writeTo p = getVA(4) should be p = getVA(isBLX(instr) ? 8 : 4); As the patch for a BLX is Arm state and the pc-offset for Arm is 8 and not 4. This accounts for the incorrect start + 0x4.

Ping. Note that the aforementioned issues are not required to be fixed for this patch. Moreover, I guess it will be more convenient if this patch is landed first.

My apologies for the delay, it has been a busy week at work and I've not had any spare time to review.

The code changes LGTM. There are a few places in the test where we could simplify with llvm-objdump --no-show-raw-insn, but this patch is big enough already. I did get some test failures although I think that this is due to the filecheck commands used and not the code-changes, details below.

I applied the patch to main (as of d07f43641f98a8e0024cf8e94ef98c7c912221d9) as I wanted to see if I could look into the other two problems. I ran into some test failures that I don't quite now whether they are my environment or whether something else has changed since the patch was submitted. In each case it looks like llvm-objdump is doing what we'd expect but It looks like the {{.*}} may be matching other parts of the disassembly, or is doing something slightly different on my system Ubuntu 18.04.

If anyone else can run to see if it is just my system, or something that isn't stable between systems, I'd be grateful to know.

My results:

Failed Tests (10):
  lld :: COFF/armnt-blx23t.test
  lld :: COFF/armnt-branch24t.test
  lld :: COFF/delayimports-armnt.yaml
  lld :: ELF/arm-fix-cortex-a8-blx.s
  lld :: ELF/arm-fix-cortex-a8-recognize.s
  lld :: ELF/arm-thumb-no-undefined-thunk.s
  lld :: ELF/arm-thumb-undefined-weak-narrow.test
  lld :: ELF/arm-thumb-undefined-weak.s
  lld :: ELF/arm-undefined-weak.s
lld/test/COFF/armnt-blx23t.test
15

I get test failures on my system.

Command Output (stderr):
--
/ssd/llvm-project/lld/test/COFF/armnt-blx23t.test:15:11: error: BEFORE: expected string not found in input
# BEFORE: c: 00 f0 00 f8 bl {{.+}} @ imm = #0
          ^
<stdin>:13:23: note: scanning from here
 a: 20 20 movs r0, #32
                      ^
<stdin>:14:2: note: possible intended match here
 c: 00 f0 00 f8 bl 0x10 <function+0xc>
 ^
lld/test/COFF/armnt-branch24t.test
13

I get test failures on my system

# BEFORE: 6: 00 f0 00 b8 b.w {{.+}} @ imm = #0
          ^
<stdin>:11:23: note: scanning from here
 4: 20 20 movs r0, #32
                      ^
<stdin>:12:2: note: possible intended match here
 6: 00 f0 00 b8 b.w 0xa <function+0x6>
 ^
lld/test/COFF/delayimports-armnt.yaml
54

I get test failures:

/ssd/llvm-project/lld/test/COFF/delayimports-armnt.yaml:57:16: error: DISASM-NEXT: expected string not found in input
# DISASM-NEXT: 00 f0 00 b8 b.w {{.+}} @ imm = #0
               ^
<stdin>:12:35: note: scanning from here
 401010: c0 f2 40 0c movt r12, #64
                                  ^
<stdin>:13:10: note: possible intended match here
 401014: 00 f0 00 b8 b.w 0x401018 <.text+0x18>
         ^
lld/test/ELF/arm-fix-cortex-a8-blx.s
30

I get a test failure

/ssd/llvm-project/lld/test/ELF/arm-fix-cortex-a8-blx.s:34:22: error: CHECK-PATCH-NEXT: expected string not found in input
// CHECK-PATCH-NEXT: 22004: b 0x21004 <{{.+}}> @ imm = #-4104
                     ^
<stdin>:11:35: note: scanning from here
00022004 <__CortexA8657417_21FFE>:
                                  ^
<stdin>:12:2: note: possible intended match here
 22004: b 0x21004 <$d.1>
 ^

My disassembly is:

Disassembly of section .text:

00021ffa <$t.2>:
   21ffa:       nop.w
   21ffe:       blx     0x22004 <__CortexA8657417_21FFE>
   22002:       bmi     0x21fae <$d.1+0xfaa>

00022004 <__CortexA8657417_21FFE>:
   22004:       b       0x21004 <$d.1>
lld/test/ELF/arm-fix-cortex-a8-recognize.s
58

I get a test failure with:

/ssd/llvm-project/lld/test/ELF/arm-fix-cortex-a8-recognize.s:62:28: error: CHECK-RELOCATABLE-NEXT: expected string not found in input
// CHECK-RELOCATABLE-NEXT: ffe: b.w {{.+}} @ imm = #-4
                           ^
<stdin>:7:12: note: scanning from here
 ffa: nop.w
           ^
<stdin>:8:2: note: possible intended match here
 ffe: b.w 0xffe <target+0x4>
 ^
lld/test/ELF/arm-thumb-no-undefined-thunk.s
22–25

I get a test failure here:

// CHECK: 200b4: {{.*}} bl 0x200b8 <_start+0x4> @ imm = #0
          ^
<stdin>:6:19: note: scanning from here
000200b4 <_start>:
                  ^
<stdin>:7:15: note: possible intended match here
 200b4: 00 f0 00 f8 bl 0x200b8 <_start+0x4>
              ^
lld/test/ELF/arm-thumb-undefined-weak-narrow.test
8

I get a test failure with a dissasembly of:

000200b4 <_start>:
   200b4: ff e7         b       0x200b6 <_start+0x2>

Not sure why that isn't matching for me.

lld/test/ELF/arm-thumb-undefined-weak.s
4

can add --no-show-raw-insn to miss out matched with {{.*}}

39–40

I see failures:

/ssd/llvm-project/lld/test/ELF/arm-thumb-undefined-weak.s:40:11: error: CHECK: expected string not found in input
// CHECK: 200b4: {{.*}} beq.w 0x200b8 <_start+0x4> @ imm = #0
          ^
<stdin>:5:1: note: scanning from here

^
<stdin>:7:15: note: possible intended match here
 200b4: 00 f0 00 80 beq.w 0x200b8 <_start+0x4>
              ^
lld/test/ELF/arm-undefined-weak.s
35–36

When I run the patch locally I get test failures, even though the disassembly is what I would expect, a branch to the next instruction

The test output is

/ssd/llvm-project/lld/test/ELF/arm-undefined-weak.s:37:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: 100100b4: b {{.+}} @ imm = #-4
               ^
<stdin>:6:19: note: scanning from here
100100b4 <_start>:
                  ^
<stdin>:7:1: note: possible intended match here
100100b4: b 0x100100b8 <_start+0x4>
^

The disassembly I get is:

100100b4 <_start>:
100100b4:       b       0x100100b8 <_start+0x4>
100100b8:       bl      0x100100bc <_start+0x8>
100100bc:       bl      0x100100c0 <_start+0xc>
100100c0:       movt    r0, #0
100100c4:       movw    r0, #0

100100c8 <$d.1>:
100100c8:       00 00 00 00     .word   0x00000000

D104907 for the update to arm-thumb-interwork-thunk.s

Will be out of the office till next Tuesday. Happy for others to approve this.

Thanks for the comprehensive report. I reproduced the failures. I'll investigate and check them the next week.

I applied the patch to main (as of d07f43641f98a8e0024cf8e94ef98c7c912221d9) as I wanted to see if I could look into the other two problems. I ran into some test failures that I don't quite now whether they are my environment or whether something else has changed since the patch was submitted. In each case it looks like llvm-objdump is doing what we'd expect but It looks like the {{.*}} may be matching other parts of the disassembly, or is doing something slightly different on my system Ubuntu 18.04.

This patch requires D104699 to work but you probably applied it without the latter.

Matt added a subscriber: Matt.Jun 28 2021, 12:31 PM
peter.smith accepted this revision.Jun 29 2021, 2:28 AM

Thanks for the update. I've updated this morning and checked that D104699 was present. The tests pass for me.

LGTM.

This revision is now accepted and ready to land.Jun 29 2021, 2:28 AM
This revision was landed with ongoing or failed builds.Jun 30 2021, 2:36 AM
This revision was automatically updated to reflect the committed changes.