[llvm-readobj/llvm-readelf] - Don't fail to dump the object if .dynsym has broken sh_link field.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=42215.

GNU readelf allows to dump the objects in that case,
but llvm-readobj/llvm-readelf reports an error and stops.

The patch fixes that.

[grimar]

• Updated the test case after rL362929.

[jhenderson]

tools/llvm-readobj/ELFDumper.cpp

1426 ↗

(On Diff #203803)

I'm a little uncomfortable with this interface. unwrapOrWarn implies that you'll get a warning or an object, yet you actually end up with both, with the latter being default constructed. However, not all objects are default constructible, meaning that this function can't be called for those types. Furthermore, the default type must be something that can be handled further on in the code correctly (imagine how easy it would be for a crash if the return type was a pointer). I think that way lies the risk of bugs. I'd suggest following the more traditional Expected behaviour, something like this:

Type T;
if (Expected<Type> E = getAType())
  T = *E;
else {
  reportWarning("some message);
  T = Type(A, B);
}

In a StringRef case, what you're doing is fine, as long as an empty StringRef makes sense further down, but that's not always the case.

tools/llvm-readobj/llvm-readobj.h

47 ↗

(On Diff #203803)

Blank line before the function, please.

EO doesn't make sense as the variable name. What does it stand for?

[grimar]

• Addressed review comments.

tools/llvm-readobj/llvm-readobj.h

47 ↗

(On Diff #203803)

This method was removed completely.

Blank line before the function, please.

This probably was fine, it was consistent with other "unwrapOr*" helpers above. Sometimes in LLVM functions are grouped together without blank lines in between.

EO doesn't make sense as the variable name. What does it stand for?

Copy paste.. Looking on the code above, initially it came from ErrorOr, but then was reused for Expected<T>

[jhenderson]

LGTM, with one remaining suggestion.

tools/llvm-readobj/llvm-readobj.h

26 ↗

(On Diff #203812)

I'd call this function warn, not warning (warning is not a verb).

llvm/trunk/test/tools/llvm-readobj/elf-broken-dynsym-link.test

+## Test that we are able to dump section headers even if the
+## .dynsym section's sh_link field is broken.
+
+## Case 1: sh_link is set to 0.
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readobj -S %t1 2>&1 | FileCheck %s --check-prefixes=LLVM,ERR
+# RUN: llvm-readelf -S %t1 2>&1 | FileCheck %s --check-prefixes=GNU,ERR
+
+# ERR: warning: invalid sh_type for string table, expected SHT_STRTAB
+
+# LLVM:      Name: .dynsym
+# LLVM-NEXT:  Type: SHT_DYNSYM
+# LLVM-NEXT:  Flags [
+# LLVM-NEXT:    SHF_ALLOC
+# LLVM-NEXT:  ]
+# LLVM-NEXT:  Address: 0x0
+# LLVM-NEXT:  Offset: 0x180
+# LLVM-NEXT:  Size: 24
+# LLVM-NEXT:  Link: 0
+
+# GNU:      Section Headers:
+# GNU-NEXT:  [Nr] Name    Type   Address          Off    Size   ES Flg Lk
+# GNU-NEXT:  [ 0]         NULL   0000000000000000 000000 000000 00 0   0
+# GNU-NEXT:  [ 1] .dynsym DYNSYM 0000000000000000 000180 000018 18 A   0
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+
+## TODO: Remove precompiled input object after fixing yaml2obj.
+##       See https://bugs.llvm.org/show_bug.cgi?id=42216.
+
+## Case 2: sh_link is set to 255, which is larger than the number of the sections.
+# RUN: llvm-readobj -S %p/Inputs/elf-broken-dynsym-link.elf-x86-64 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=LLVM2,ERR2
+# RUN: llvm-readelf -S %p/Inputs/elf-broken-dynsym-link.elf-x86-64 2>&1 \
+# RUN:   | FileCheck %s --check-prefixes=GNU2,ERR2
+
+# ERR2: warning: invalid section index
+
+# LLVM2:      Name: .dynsym
+# LLVM2-NEXT:  Type: SHT_DYNSYM
+# LLVM2-NEXT:  Flags [
+# LLVM2-NEXT:    SHF_ALLOC
+# LLVM2-NEXT:  ]
+# LLVM2-NEXT:  Address: 0x0
+# LLVM2-NEXT:  Offset: 0x180
+# LLVM2-NEXT:  Size: 2
+# LLVM2-NEXT:  Link: 255
+
+# GNU2:      Section Headers:
+# GNU2-NEXT:  [Nr] Name    Type   Address          Off    Size   ES Flg Lk
+# GNU2-NEXT:  [ 0]         NULL   0000000000000000 000000 000000 00 0   0
+# GNU2-NEXT:  [ 1] .dynsym DYNSYM 0000000000000000 000180 000002 18 A   255

llvm/trunk/test/tools/yaml2obj/explicit-dynsym-no-dynstr.yaml

 ## Check we do not crash/assert when .dynsym is specified
 ## explicitly, but .dynstr is not present.
 
 # RUN: yaml2obj %s -o %t
-# RUN: not llvm-readelf --section-headers %t 2>&1 | FileCheck %s
+# RUN: llvm-readelf --section-headers %t | FileCheck %s
 
-## TODO: Check that .dynsym has Link field set to 0.
-##       GNU readelf is able to dump sections headers,
-##       but llvm-readelf reports an error below too early.
-##       See https://bugs.llvm.org/show_bug.cgi?id=42215.
+## Check that .dynsym has Link field set to 0.
 
-# CHECK: error: invalid sh_type for string table, expected SHT_STRTAB
+# CHECK:      Section Headers:
+# CHECK-NEXT:  [Nr] Name    Type   Address          Off    Size   ES Flg Lk
+# CHECK-NEXT:  [ 0]         NULL   0000000000000000 000000 000000 00 0   0
+# CHECK-NEXT:  [ 1] .dynsym DYNSYM 0000000000000000 000180 000018 18 A   0
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
   - Name: .dynsym
     Type: SHT_SYMTAB

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

case ELF::SHT_SYMTAB:
         DotSymtabSec = &Sec;
       break;
     case ELF::SHT_DYNSYM:
       if (!DynSymRegion.Size) {
         DynSymRegion = createDRIFrom(&Sec);
         // This is only used (if Elf_Shdr present)for naming section in GNU
         // style
         DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
-        DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
+
+        if (Expected<StringRef> E = Obj->getStringTableForSymtab(Sec))
+          DynamicStringTable = *E;
+        else
+          warn(E.takeError());
       }
       break;
     case ELF::SHT_SYMTAB_SHNDX:
       ShndxTable = unwrapOrError(Obj->getSHNDXTable(Sec));
       break;
     case ELF::SHT_GNU_versym:
       if (!SymbolVersionSection)
         SymbolVersionSection = &Sec;

llvm/trunk/tools/llvm-readobj/llvm-readobj.h

 namespace llvm {
   namespace object {
     class RelocationRef;
   }
 
   // Various helper functions.
   LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
   void reportWarning(Twine Msg);
+  void warn(llvm::Error Err);
   void error(std::error_code EC);
   void error(llvm::Error EC);
   template <typename T> T error(llvm::Expected<T> &&E) {
     error(E.takeError());
     return std::move(*E);
   }
 
   template <class T> T unwrapOrError(ErrorOr<T> EO) {

llvm/trunk/tools/llvm-readobj/llvm-readobj.cpp

LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
   exit(1);
 }
 
 void reportWarning(Twine Msg) {
   errs() << "\n";
   WithColor::warning(errs()) << Msg << "\n";
 }
 
+void warn(Error Err) {
+  handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
+    reportWarning(EI.message());
+  });
+}
+
 void error(Error EC) {
   if (!EC)
     return;
   handleAllErrors(std::move(EC),
                   [&](const ErrorInfoBase &EI) { reportError(EI.message()); });
 }
 
 void error(std::error_code EC) {