[PATCH] D71453: [llvm-objdump] Print target address instead of call/j/br pc-rel immediate

Mon Dec 16 01:36:38 PST 2019

jhenderson added a comment.

I have no issues with attempting to improve GNU compatibility, but please see my inline comments, as you appear to have degraded some of the experience for one feature that is not present in GNU objdump. Also, I'm somewhat surprised that this issue doesn't affect other targets. Do they already do the right thing?

================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1341-1342

-  // If the label has already been resolved to an immediate offset (say, when
-  // we're running the disassembler), just print the immediate.
   if (Op.isImm()) {
----------------
Rather than removing this comment, I think you should change it to say why you do nothing here.

================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1344
   if (Op.isImm()) {
-    O << "#" << formatImm(Op.getImm() * 4);
     return;
----------------
Rather than removing the syntax highlighting, I think it would be preferable to keep it, but perhaps in a different colour? @seiya added this as part of the GSOC work last summer, so he may be best placed to suggest an alternative.

Same comment applies elsewhere.

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

https://reviews.llvm.org/D71453