[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155107#4504667 <https://reviews.llvm.org/D155107#4504667>, @jasonmolenda wrote:

> Isn't it better to print branches within a function as an offset, given that our disassembly format by default lists the offset of each instruction.  So instead of looking for a 6-digit long hex address, you're looking for a decimal offset in the output?  I'm not sure if you disagree with my opinion, or if you have an example where it is clearer to have an address that I'm not seeing.

llvm-objdump calls setPrintImmAsAddress(true). I think we should do the same. I also think that it looks better. @clayborg here is some riscv32 disassembly with and without the change:

with:

  (lldb) dis -n factorial
  factrv32`factorial:
      0x1047c <+0>:  addi   sp, sp, -32
      0x1047e <+2>:  sw     ra, 28(sp)
      0x10480 <+4>:  sw     s0, 24(sp)
      0x10482 <+6>:  addi   s0, sp, 32
      0x10484 <+8>:  sw     a0, -16(s0)
      0x10488 <+12>: lw     a0, -16(s0)
      0x1048c <+16>: li     a1, 1
      0x1048e <+18>: beq    a0, a1, 0x10496
      0x10492 <+22>: j      0x104a4
      0x10496 <+26>: j      0x1049a
      0x1049a <+30>: li     a0, 1
      0x1049c <+32>: sw     a0, -12(s0)
      0x104a0 <+36>: j      0x104c2
      0x104a4 <+40>: lw     a0, -16(s0)
      0x104a8 <+44>: sw     a0, -20(s0)
      0x104ac <+48>: addi   a0, a0, -1
      0x104ae <+50>: jal    0x1047c
      0x104b0 <+52>: mv     a1, a0
      0x104b2 <+54>: lw     a0, -20(s0)
      0x104b6 <+58>: mul    a0, a0, a1
      0x104ba <+62>: sw     a0, -12(s0)
      0x104be <+66>: j      0x104c2
      0x104c2 <+70>: lw     a0, -12(s0)
      0x104c6 <+74>: lw     ra, 28(sp)
      0x104c8 <+76>: lw     s0, 24(sp)
      0x104ca <+78>: addi   sp, sp, 32
      0x104cc <+80>: ret    

without:

  factrv32`factorial:
      0x1047c <+0>:  addi   sp, sp, -32
      0x1047e <+2>:  sw     ra, 28(sp)
      0x10480 <+4>:  sw     s0, 24(sp)
      0x10482 <+6>:  addi   s0, sp, 32
      0x10484 <+8>:  sw     a0, -16(s0)
      0x10488 <+12>: lw     a0, -16(s0)
      0x1048c <+16>: li     a1, 1
      0x1048e <+18>: beq    a0, a1, 8
      0x10492 <+22>: j      18
      0x10496 <+26>: j      4
      0x1049a <+30>: li     a0, 1
      0x1049c <+32>: sw     a0, -12(s0)
      0x104a0 <+36>: j      34
      0x104a4 <+40>: lw     a0, -16(s0)
      0x104a8 <+44>: sw     a0, -20(s0)
      0x104ac <+48>: addi   a0, a0, -1
      0x104ae <+50>: jal    -50
      0x104b0 <+52>: mv     a1, a0
      0x104b2 <+54>: lw     a0, -20(s0)
      0x104b6 <+58>: mul    a0, a0, a1
      0x104ba <+62>: sw     a0, -12(s0)
      0x104be <+66>: j      4
      0x104c2 <+70>: lw     a0, -12(s0)
      0x104c6 <+74>: lw     ra, 28(sp)
      0x104c8 <+76>: lw     s0, 24(sp)
      0x104ca <+78>: addi   sp, sp, 32
      0x104cc <+80>: ret    

Addresses 0x10492 and 0x10496 are jumps. It's much easier, IMO, to see what the targets are with the change than without.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155107/new/

https://reviews.llvm.org/D155107