[llvm-readobj] - Report warnings instead of errors for broken relocations.

[grimar]

Summary

This is a follow-up for https://reviews.llvm.org/D74545.

It adds test cases for each incorrect case returned in getRelocationTarget.

[grimar]

Will check something.

[grimar]

• Improved how errors looks like.
• Fixed one more test case.

[MaskRay]

llvm/test/Object/invalid.test

420 ↗

(On Diff #244607)

We probably should split invalid.test. For example, segments related errors and sections related errors can be separated.

llvm/test/tools/llvm-readobj/ELF/relocations-errors.test

2

Maybe relocation-errors.test?

llvm/tools/llvm-readobj/ELFDumper.cpp

3327

Target->first

5746

Target->second

[grimar]

• Addressed review comments.

llvm/test/Object/invalid.test

420 ↗

(On Diff #244607)

It feels that some of the tests might go to the corresponding tools test folders.
(e.g. docnum 21 below)

[jhenderson]

llvm/test/Object/invalid.test

420 ↗

(On Diff #244607)

Better yet would be to replace those tests which are testing library code with gtest unit tests to test the interfaces directly instead (and then additional tests in the tools folder to show the tools correctly handle the respective places properly).

llvm/test/tools/llvm-readobj/ELF/relocation-errors.test

1 ↗

(On Diff #244868)

Delete "do"

55–58 ↗

(On Diff #244868)

Seems like this overlaps with reloc-no-sym.test? Could we get rid of one or the other?

69 ↗

(On Diff #244868)

relocation in a section...

70 ↗

(On Diff #244868)

is neither SHT_SYMTAB nor SHT_DYNSYM

[grimar]

• Addressed review comments.

llvm/test/tools/llvm-readobj/ELF/relocation-errors.test

55–58 ↗

(On Diff #244868)

This one is a bit different: here we have a SHT_REL section.
(I can't test an addend here, like reloc-no-sym.test do).

And the intention to have it also was to show the order of warnings/normal relocations.
I.e. I wanted to have at least one valid relocation in the middle and
"relocation against a symbol with index 0" works good as it is related to the context and the
current code.

So, I'd keep both. (btw, reloc-no-sym.test seems lacks GNU test and SHT_REL test and might need to be improved).

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/relocation-errors.test

47 ↗

(On Diff #245094)

no a symbol -> no symbol
index of -> index

llvm/tools/llvm-readobj/ELFDumper.cpp

3324–3325

I'm not sure this error message quite makes sense. To me, I read it as dumping the relocation at file offset 0x1234 (i.e. where the relocation is physically stored in the file), whereas the offset is the section-relative offset.

Perhaps this error should print the index of the relocation and its section, i.e. something like "unable to print the relocation with index 4 in .rela.text: ...". I'd also recommend the phrasing I've used there regarding "unable to print" instead of "error dumping".

Same comment goes for LLVM style.

By the way, would it not make sense to print something like "<?>" or "<corrupt>" etc for the invalid values?

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

3324–3325

By the way, would it not make sense to print something like "<?>" or "<corrupt>" etc for the invalid values?

Yes. I had an experiment about refactoring the getRelocationTarget method (used getFullSymbolName for STT_SECTION symbols path as it has very similar code). As a result I had 2 warnings of this patch gone and "<?>" displayed.
I think we might want both: i.e. to show a warning about what is wrong about the object parsed and to dump a relocation, using "<?>" if needed. It seems to be a good possible follow-up change.

Perhaps this error should print the index of the relocation and its section, i.e. something like "unable to print the relocation with index 4 in .rela.text: ...". I'd also recommend the phrasing I've used there regarding "unable to print" instead of "error dumping".

I've applied your suggestion with a change: I've used a section index instead of a section name in the error message:
this is more consistent with our others errors. Looks fine?

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

3324–3325

Looks fine, if a little verbose. Do you think something like "unable to print relocation 1 in section 2 ..." would be sufficient?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

3324–3325

It sounds fine to me. I'll apply.

[grimar]

• Addressed review comment.

[jhenderson]

LGTM.

llvm/include/llvm/Object/ELF.h

 }
 
 template <class ELFT>
 Expected<StringRef>
 ELFFile<ELFT>::getStringTableForSymtab(const Elf_Shdr &Sec,
                                        Elf_Shdr_Range Sections) const {
 
   if (Sec.sh_type != ELF::SHT_SYMTAB && Sec.sh_type != ELF::SHT_DYNSYM)
-    // TODO: this error is untested.
     return createError(
         "invalid sh_type for symbol table, expected SHT_SYMTAB or SHT_DYNSYM");
   auto SectionOrErr = object::getSection<ELFT>(Sections, Sec.sh_link);
   if (!SectionOrErr)
     return SectionOrErr.takeError();
   return getStringTable(*SectionOrErr);
 }
 

llvm/test/tools/llvm-readobj/ELF/relocations-errors.test

+## Check how do we report warnings when dumping an object with broken relocations.
+
+# RUN: yaml2obj %s -o %t64
+# RUN: llvm-readobj --relocations %t64 2>&1 | FileCheck %s -DFILE=%t64 --check-prefix=LLVM
+# RUN: llvm-readelf --relocations %t64 2>&1 | FileCheck %s -DFILE=%t64 --check-prefix=GNU
+
+# LLVM:      Relocations [
+# LLVM-NEXT:   Section (3) .rel.text {
+# LLVM-NEXT: warning: '[[FILE]]': unable to access section [index 6] data at 0x17e7e7e8b0: offset goes past the end of file
+# LLVM-NEXT:     0x2 R_X86_64_NONE - 0x0
+# LLVM-NEXT: warning: '[[FILE]]': invalid section index: 255
+# LLVM-NEXT: warning: '[[FILE]]': a section [index 2] has an invalid sh_name (0xfefefefe) offset which goes past the end of the section name string table
+# LLVM-NEXT:   }
+# LLVM-NEXT:   Section (4) .rela.text {
+# LLVM-NEXT: warning: '[[FILE]]': invalid sh_type for symbol table, expected SHT_SYMTAB or SHT_DYNSYM
+# LLVM-NEXT:   }
+# LLVM-NEXT: ]
+
+# GNU:       Relocation section '.rel.text' at offset 0x41 contains 5 entries:
+# GNU-NEXT:      Offset             Info             Type               Symbol's Value  Symbol's Name
+# GNU-NEXT:  warning: '[[FILE]]': unable to access section [index 6] data at 0x17e7e7e8b0: offset goes past the end of file
+# GNU-NEXT:  0000000000000002  0000000000000000 R_X86_64_NONE
+# GNU-NEXT:  warning: '[[FILE]]': invalid section index: 255
+# GNU-NEXT:  warning: '[[FILE]]': a section [index 2] has an invalid sh_name (0xfefefefe) offset which goes past the end of the section name string table
+# GNU-EMPTY:
+# GNU-NEXT:  Relocation section '.rela.text' at offset 0x91 contains 1 entries:
+# GNU-NEXT:      Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+# GNU-NEXT:  warning: '[[FILE]]': invalid sh_type for symbol table, expected SHT_SYMTAB or SHT_DYNSYM
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Content: '00'
+  - Name:   .foo
+    Type:   SHT_PROGBITS
+    ShName: 0xFEFEFEFE
+  - Name: .rel.text
+    Type: SHT_REL
+    Info: .text
+    Relocations:
+## Case 1: There is no a symbol with index of 0xFEFEFEFE. Also, check
+##         that warnings we report are uniqified.
+      - Offset: 0x0
+        Symbol: 0xFEFEFEFE
+        Type:   R_X86_64_NONE
+      - Offset: 0x1
+        Symbol: 0xFEFEFEFE
+        Type:   R_X86_64_NONE
+## Case 2: Test that no warning is reported for a relocation against a symbol with index 0.
+      - Offset: 0x2
+        Symbol: 0
+        Type:   R_X86_64_NONE
+## Case 3: Test a relocation against a section symbol that has an invalid
+##         section index (larger than the number of sections).
+      - Offset: 0x3
+        Symbol: .sec.symbol1
+        Type:   R_X86_64_NONE
+## Case 4: Test a relocation against a section symbol that has an invalid
+##         sh_name offset that goes past the end of the section name string table.
+      - Offset: 0x4
+        Symbol: .sec.symbol2
+        Type:   R_X86_64_NONE
+## Case 5: Test a relocation in the section that is linked to a symbol table that
+##         has a section type that is not a SHT_SYMTAB or SHT_DYNSYM.
+##         In this case the code fails to find a corresponding symbol string table.
+  - Name: .rela.text
+    Type: SHT_RELA
+    Info: .text
+    Link: .fake.symtab
+    Relocations:
+      - Offset: 0x5
+        Symbol: symbol
+        Type:   R_X86_64_NONE
+  - Name:    .fake.symtab
+    Type:    SHT_PROGBITS
+    EntSize: 24
+    Size:    48
+Symbols:
+  - Name:    symbol
+    Section: .text
+    Value:   0
+  - Name:  .sec.symbol1
+    Type:  STT_SECTION
+    Index: 0xFF
+  - Name:  .sec.symbol2
+    Type:  STT_SECTION
+    Index: 0x2

llvm/tools/llvm-readobj/ELFDumper.cpp

template <class ELFT> void GNUStyle<ELFT>::printGroupSections(const ELFO *Obj) {
 
   if (V.empty())
     OS << "There are no section groups in this file.\n";
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printRelocation(const ELFO *Obj, const Elf_Shdr *SymTab,
                                      const Elf_Rela &R, bool IsRela) {
-  const typename ELFT::Sym *Sym;
-  std::string Name;
-  std::tie(Sym, Name) = unwrapOrError(
-      this->FileName, this->dumper()->getRelocationTarget(SymTab, R));
-  printRelocation(Obj, Sym, Name, R, IsRela);
+  Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
+      this->dumper()->getRelocationTarget(SymTab, R);
+  if (!Target)
+    this->reportUniqueWarning(Target.takeError());
+  else
+    printRelocation(Obj, /*Sym=*/(*Target).first, /*Name=*/(*Target).second, R,
+                    IsRela);
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printRelocation(const ELFO *Obj, const Elf_Sym *Sym,
                                      StringRef SymbolName, const Elf_Rela &R,
                                      bool IsRela) {
   // First two fields are bit width dependent. The rest of them are fixed width.
   unsigned Bias = ELFT::Is64Bits ? 8 : 0;
   Field Fields[5] = {0, 10 + Bias, 19 + 2 * Bias, 42 + 2 * Bias, 53 + 2 * Bias};
for (const Elf_Rela &R :
       printRelocation(Obj, R, SymTab);
     break;
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printRelocation(const ELFO *Obj, Elf_Rela Rel,
                                       const Elf_Shdr *SymTab) {
-  const typename ELFT::Sym *Sym;
-  std::string TargetName;
-  std::tie(Sym, TargetName) = unwrapOrError(
-      this->FileName, this->dumper()->getRelocationTarget(SymTab, Rel));
+  Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
+      this->dumper()->getRelocationTarget(SymTab, Rel);
+  if (!Target) {
+    this->reportUniqueWarning(Target.takeError());
+    return;
+  }
 
+  std::string TargetName =  (*Target).second;
   SmallString<32> RelocName;
   Obj->getRelocationTypeName(Rel.getType(Obj->isMips64EL()), RelocName);
 
   if (opts::ExpandRelocs) {
     DictScope Group(W, "Relocation");
     W.printHex("Offset", Rel.r_offset);
     W.printNumber("Type", RelocName, (int)Rel.getType(Obj->isMips64EL()));
     W.printNumber("Symbol", !TargetName.empty() ? TargetName : "-",