[PATCH] D100489: [llvm-objdump] Fix dumping dynamic relative relocations for SHT_REL

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test:9
+
+# CHECK:      file format elf32-i386
+# CHECK-LABEL: DYNAMIC RELOCATION RECORDS
----------------
arichardson wrote:
> jhenderson wrote:
> > I don't think this line is useful?
> I agree. I simply copied it from `llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test`. Should I drop it there too?
It probably wouldn't hurt (just make sure that somewhere there is a lit test that shows that the file format line is printed, so that we don't lose coverage!)


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test:31
+  - Name:            .rel.dyn
+    Type:            SHT_REL
+    Flags:           [ SHF_ALLOC ]
----------------
You probably should have `Link: .dynsym` here, since these are dynamic relocations (by default, it'll use the static symbol table).


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test:61-63
+Symbols:
+  - Name:    foo
+    Section: .data
----------------
arichardson wrote:
> jhenderson wrote:
> > I think you can omit the static symbol?
> Unfortunately that gives me a `yaml2obj` error when using `Symbol: foo` inside `Relocations:`.
> I haven't looked at the yaml2obj code, but it seems like we would need to either hardcode dynamic relocation section names or add another attribute to `Relocations` to indicate that it should look in the dynamic symbol list instead. Alternatively, we could make that decision based on the file type (ET_REL uses `Symbols`, all other use `DynamicSymbols`? 
Ah okay, thanks. Try the `Link:` suggestion above. If that doesn't work, I think adding a FIXME/TODO comment of some kind here would be a good idea.

@grimar, any thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100489