[PATCH] D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the example, they helped me understand much better what you're trying to achieve and +1 to the idea definitely. In a weird coincidence, one of our toolchain team only last week demo'ed to the team a script to make comparing diassembly much less noisy. This option may help with some of that.



================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:202
+
+  When disassembling, symbolize a branch target operand to print a label instead of real address.
+
----------------
instead of real -> instead of a real


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:202-204
+  When disassembling, symbolize a branch target operand to print a label instead of real address.
+
+  When printing a PC-relative global symbol reference, print it as an offset from the leading symbol.
----------------
jhenderson wrote:
> instead of real -> instead of a real
It might be worth including a couple of simple examples of what gets printed with the option. I found the example you posted in the patch description a much clearer way of expressing this compared to just words (you obviously should still keep the words).


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:206
+
+  Only works with --x86-asm-syntax=Intel and an executable binary.
+
----------------
MaskRay wrote:
> You probably want to mean a linked image (not a relocatable object file).
> 
> Capitalized `Intel` isn't supported. att is much more common and --x86-asm-syntax=intel is rare. As a related fact, LLVM's Intel syntax assembler gets less scrutiny and tends to have some problems.
Is there anything fundamentally preventing this from working with the default assembly syntax or relocatable objects? Or is it just that you haven't implemented it yet?


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:208
+
+
 .. option:: --triple=<string>
----------------
Nit: delete second blank line.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:68
 
+  /// If true, symbolize branch target operands and memory reference operands.
+  bool SymbolizeOperands = false;
----------------
"symbolize branch target and memory reference operands"

(no need to repeat operands twice this way)


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-symbolize-operands.test:1
+// RUN: llvm-objdump -d --symbolize-operands --x86-asm-syntax=intel --no-show-raw-insn --no-leading-addr  %p/Inputs/disassemble-symbolize-operands.exe.elf-x86_64 | FileCheck %s
+
----------------
MaskRay wrote:
> Please avoid a checked-in executable. Its content is difficult to inspect.
> See `X86/elf-disassemble-symbol-references.yaml` for a yaml2obj example.
You probably want to use FileCheck's --match-full-lines to show that you are capturing all the content on the line and that there isn't any additional text before/after the thing you check for.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-symbolize-operands.test:4-10
+// CHECK: <_start>:
+// CHECK: <L1>:
+// CHECK: cmp	eax, dword ptr <g>
+// CHECK: jge <L0>
+// CHECK: call <foo>
+// CHECK: jmp <L1>
+// CHECK: <L0>:
----------------
Consider using CHECK-NEXT, rather than CHECK, if possible.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1580-1583
+  // So far only supports X86. Other target like Arm64 may have data embedded in
+  // the text section.
+  if (!STI->getTargetTriple().isX86())
+    return;
----------------
I don't think X86 is exempt from potentially having data embedded in the text section. I think I've seen jump tables that are, for example.

It's not clear to me, why a target having potential data embedding is an issue?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1594
+  uint64_t Index = Start;
+  while (Index < End) {
+    // Disassemble a real instruction and record function-local branch labels.
----------------
I'd be slightly concerned by this additional pass over the disassembly, due to performance reasons, but I guess it's behind a switch, so it's not that big a deal. Plus, I can't think how you'd identify where the labels should go otherwise, since they might appear before the reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84191