[PATCH] D62972: [ELF] Treat dynamic tag values as virtual addresses instead of offsets

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: grimar, rupprecht, MaskRay.
jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:1
+--- !ELF
+# Show that llvm-objdump can dump dynamic relocations.
----------------
Move this line to immediately before the rest of the YAML.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:2-4
+# Show that llvm-objdump can dump dynamic relocations.
+# Specifically, we are checking that the tags DT_RELA, DT_REL and DT_JMPREL
+# properly identify relocation tables.
----------------
In many of the newer tests in the llvm tools, we've been using '##' to distinguish comments from RUN/CHECK commands. It might be nice if you added those.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:9
+
+# CHECK: 0000000000000000 R_X86_64_RELATIVE *ABS*
+# CHECK: 0000000000000000 R_X86_64_JUMP_SLOT bar
----------------
As this is the first and only test for --dynamic-relocs, could you check the full output here (including relevant headers etc if any), please.

Also, you should make your checks CHECK-NEXT, and add something to show that there are no other relocations printed (in case later lines produce garbage or similar). This might be most easily achieved by doing an --implicit-check-not=R_X86 or similar.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:11
+# CHECK: 0000000000000000 R_X86_64_JUMP_SLOT bar
+# CHECK: 0000000000000008 R_X86_64_RELATIVE foo
+
----------------
RELATIVE + symbol doesn't make sense. Probably just change the type to something else (e.g. R_X86_64_NONE).


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:27
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_WRITE, SHF_ALLOC ]
+    Address:      0x100100
----------------
Nit, this is formatted differently to `[SHF_ALLOC]` above (i.e. spacing is different). Please could you make it consistent.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:29
+    Address:      0x100100
+    AddressAlign: 0x1000
+  - Name:         .rela.dyn
----------------
I'm not sure an AddressAlign of 0x1000 makes sense if the address is only aligned to 0x100. Not sure it matters that much, but it might be nice to avoid surprises. Same comment applies in other places later on.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:47
+    Link:         .dynsym
+    Flags:        [ SHF_ALLOC ]
+    Relocations:
----------------
See above re. formatting.


================
Comment at: test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:50
+      - Offset: 0
+        Symbol: 2  # bar
+        Type:   R_X86_64_JUMP_SLOT
----------------
Nit: double-space between '2' and '#'


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

https://reviews.llvm.org/D62972