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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson 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
----------------
jhenderson wrote:
> 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!)
Looks like it's tested by llvm/test/tools/llvm-objdump/ELF/file-headers.test, so I have dropped it in this version.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test:61-63
+Symbols:
+  - Name:    foo
+    Section: .data
----------------
jhenderson wrote:
> 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?
Thanks, adding `Link:` allows me to drop `Symbols:`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100489