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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:206
+
+  Only works with a linked image.
+
----------------
You probably want to mention that this option currently only supports X86 output.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:211
+
+.. code-block:: none
+    cmp eax, dword ptr [rip + 4112]
----------------
As this is associated with the option, it needs indenting with a couple of spaces, just like the text describing the option. Same goes below. If you take a look at the output, you'll see that adding a couple of spaces before should cause the code to be shifted slightly to the right.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbololize-operands.yaml:17
+
+# INTEL:     <_start>:
+# INTEL-NEXT:   push rax
----------------
Nit: add one more space (the '<' doesn't line up with those below currently).


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbololize-operands.yaml:9
+# ATT-NEXT: <L1>:
+# ATT-NEXT: cmpl	, %eax <g>
+# ATT-NEXT: jge <L0>
----------------
hoyFB wrote:
> jhenderson wrote:
> > What's with the spaces before the ',' here.
> It is for the alignment of the first operand which isn't printed with the symbolization.
So I think we want to fix this, but I a) am not familiar enough with the `MCInstPrinter` etc code to have a good recommendation, and b) think it could be deferred to a follow-up patch. Perhaps @MaskRay has a suggestion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84191