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

[arichardson]

Summary

Previously printing R_386_RELATIVE relocations would trigger
error: can't read an entry at 0x40: it goes past the end of the section (0x40)
I found this while writing a test case for LLD (D100490)

[MaskRay]

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

2

This looks verbose.

If you just keep things as in test/tools/llvm-readelf/ELF/reloc-types-elf-i386.test, does it work?
llvm-readobj generally has better support for ELF dumping and you may take inspiration from its tests.

136

foo is not needed.

[arichardson]

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

2

Yes, I should have reduced this test input further, right now it's just and obj2yaml of an lld test with a few obvious lines removed. Will remove everything that's not needed for the relative relocation tomorrow.

[arichardson]

• simplify test case

[arichardson]

Improve test

[arichardson]

Hopefully the new test is better?

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

2

I could add a RELATIVE reloc to a .o file as in test/tools/llvm-readelf/ELF/reloc-types-elf-i386.test, but I thought using a dynamic relocation like llvm/test/tools/llvm-readobj/ELF/dynamic-reloc.test makes more sense.
The latest version also checks both relocations with and without a symbol (the same as llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test)

[arichardson]

• remove debug RUN: line

[arichardson]

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

136

I've kept foo to make this test closer to llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test and test both relocations with and without a symbol.

[jhenderson]

The patch title tags llvm-readobj, but this is an llvm-objdump change.

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

2–4

I don't think you need the "Previously ..." sentence at all. What you probably do want to say is that this test shows that Elf_Rel relocations can be dumped for both cases with and without a corresponding symbol, something like the inline edit.

10

I don't think this line is useful?

12–13

Nit suggestion to make things line up nicely.

62–64

I think you can omit the static symbol?

[arichardson]

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

10

I agree. I simply copied it from llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test. Should I drop it there too?

62–64

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?

[jhenderson]

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

10

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!)

32

You probably should have Link: .dynsym here, since these are dynamic relocations (by default, it'll use the static symbol table).

62–64

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?

[arichardson]

Address review feedback from @jhenderson

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

10

Looks like it's tested by llvm/test/tools/llvm-objdump/ELF/file-headers.test, so I have dropped it in this version.

62–64

Thanks, adding Link: allows me to drop Symbols:.

[jhenderson]

Thanks. LGTM, but please give @MaskRay a chance to confirm he's happy. Also, probably worth updating the description to mention the tidying/fixing up of elf-dynamic-relcos.test too. Alternatively, make that change in a different commit.

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test

+## Show that llvm-objdump can dump dynamic relocations for Elf_Rel relocations
+## Previously printing the R_386_RELATIVE relocations would trigger
+## error: can't read an entry at 0x40: it goes past the end of the section (0x40)
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump --dynamic-reloc %t | FileCheck %s
+# RUN: llvm-objdump -R %t | FileCheck %s
+
+# CHECK:      file format elf32-i386
+# CHECK-LABEL: DYNAMIC RELOCATION RECORDS
+# CHECK-NEXT: 00000030 R_386_RELATIVE *ABS*
+# CHECK-EMPTY
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_386
+ProgramHeaders:
+  - Type:     PT_LOAD
+    FirstSec: .rel.dyn
+    LastSec:  .got
+  - Type:     PT_DYNAMIC
+    VAddr:    0x8
+    FirstSec: .dynamic
+    LastSec:  .dynamic
+Sections:
+  - Name:            .rel.dyn
+    Type:            SHT_REL
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x4
+    Relocations:
+      - Offset:          0x30
+        Type:            R_386_RELATIVE
+  - Name:            .dynamic
+    Type:            SHT_DYNAMIC
+    Flags:           [ SHF_ALLOC ]
+    Link:            .dynstr
+    AddressAlign:    0x4
+    Entries:
+      - Tag:             DT_REL
+        Value:           0x0
+      - Tag:             DT_RELSZ
+        Value:           0x10
+      - Tag:             DT_RELENT
+        Value:           0x8
+      - Tag:             DT_RELCOUNT
+        Value:           0x1
+      - Tag:             DT_NULL
+        Value:           0x0
+  - Name:            .got
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x4
+    Content:         '80110000'
+DynamicSymbols: []

llvm/tools/llvm-objdump/ELFDump.cpp

static Error getRelocationValueString(const ELFObjectFile<ELFT> *Obj,
   //
   // In SHT_REL case we would need to read the addend from section data.
   // GNU objdump does not do that and we just follow for simplicity atm.
   bool Undef = false;
   if ((*SecOrErr)->sh_type == ELF::SHT_RELA) {
     const typename ELFT::Rela *ERela = Obj->getRela(Rel);
     Addend = ERela->r_addend;
     Undef = ERela->getSymbol(false) == 0;
-  } else if ((*SecOrErr)->sh_type != ELF::SHT_REL) {
+  } else if ((*SecOrErr)->sh_type == ELF::SHT_REL) {
+    const typename ELFT::Rel *ERel = Obj->getRel(Rel);
+    Undef = ERel->getSymbol(false) == 0;
+  } else {
     return make_error<BinaryError>();
   }
 
   // Default scheme is to print Target, as well as "+ <addend>" for nonzero
   // addend. Should be acceptable for all normal purposes.
   std::string FmtBuf;
   raw_string_ostream Fmt(FmtBuf);