[PATCH] D68066: [llvm-objdump] Further rearrange llvm-objdump sections for compatability

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D68066#1687377 <https://reviews.llvm.org/D68066#1687377>, @justice_adams wrote:

> @rupprecht A bit of a beginner to LLVM, but why add the suppression for DynamicRelocations when `--disassemble`  is provided?


The LLVM binutils, like llvm-objdump, aim to produce output identical to the equivalent GNU binutils (with some exceptions for various specific reasons). This change aims to improve that compatibility. It could be argued however, that the dynamic relocations change should be put in a separate change, since it's a slightly different issue.



================
Comment at: lld/test/ELF/mips-32.s:29
+# SYM: 00011000  .data  00000000 v1
+
+
----------------
Delete the extra blank line.


================
Comment at: lld/test/ELF/mips-got16-relocatable.s:25-26
+# SO: SYMBOL TABLE
+# SO: 00020{{0*}}[[D1:[0-9a-f]+]] .data {{0+}} data
+# SO: 00020{{0*}}[[D2:[0-9a-f]+]] .data {{0+}} data
+
----------------
What's the purpose of the new `00020{{0*}}`? I'm guessing the address printed by the disassembly is not the exact value? Could this be improved using some extra switches, so that the pattern is robust to future changes, e.g. different base addresses?


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-relocs.test:1
+## Show that --disassemble + --dynamic-reloc prints relocations inline and
+## does not dump the relocation sections.
----------------
This test doesn't make any checks for relocations being printed inline as far as I can see.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-relocs.test:56
+      - Offset: 0
+        Symbol: 2 # bar
+        Type:   R_X86_64_JUMP_SLOT
----------------
Is this needed any more? I know @grimar has been making various improvements to yaml2obj to fix things like this.


================
Comment at: llvm/test/tools/llvm-objdump/all-headers.test:15
 # CHECK: Dynamic Section:
+# CHECK:   INIT                 0x00000000000006a0
 # CHECK: Sections:
----------------
What's the point of this line?


================
Comment at: llvm/test/tools/llvm-objdump/all-headers.test:34
 # ARCHIVE: Dynamic Section:
+# ARCHIVE:   INIT                 0x00000000000006a0
 # ARCHIVE: Sections:
----------------
Ditto


================
Comment at: llvm/test/tools/llvm-objdump/all-headers.test:44
 # ARCHIVE: Dynamic Section:
+# ARCHIVE:   INIT                 0x00000000000006a0
 # ARCHIVE: Sections:
----------------
Ditto


================
Comment at: llvm/test/tools/llvm-objdump/headers-ordering.test:1
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objdump --all-headers --full-contents --dwarf=frames \
----------------
I'd rename the file "output-ordering.test" - it tests things other than just headers.


================
Comment at: llvm/test/tools/llvm-objdump/headers-ordering.test:2-3
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objdump --all-headers --full-contents --dwarf=frames \
+# RUN:   --reloc %t.o | FileCheck %s --check-prefixes=CHECK,RELOC
+# RUN: llvm-objdump --all-headers --full-contents --dwarf=frames \
----------------
I think it would be better to be explicit with all the switches rather than use --all-headers. It would help figure out what output to actually expect.


================
Comment at: llvm/test/tools/llvm-objdump/headers-ordering.test:17
+# CHECK: Dynamic Section:
+# CHECK:   INIT                 0x00000000000006a0
+## Section headers (h)
----------------
Do you need this?


================
Comment at: llvm/test/tools/llvm-objdump/headers-ordering.test:18
+# CHECK:   INIT                 0x00000000000006a0
+## Section headers (h)
+# CHECK: Sections:
----------------
-h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68066