[Object/llvm-readelf/llvm-readobj] - Improve error reporting when e_shstrndx is broken.

[grimar]

Summary

When e_shstrndx is broken, it is impossible to get a section name.
In this patch I improved the error message we show and
added tests for Object and for llvm-readelf/llvm-readobj

Message was changed in two places:

• llvm-readelf/llvm-readobj previously used a code from Object/ELF.h,

now they have a modified version of it (it has less checks and allows
dumping broken things).

• Code in Object/ELF.h is still used for generic cases.

[jhenderson]

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

Aside from that, do we have a way yet of testing the e_shstrndx == SHN_XINDEX case yet for this error? I think that deserves its own test case once we do, so probably should have a TODO note somewhere around the relevant bits of code.

include/llvm/Object/ELF.h

471 ↗

(On Diff #209788)

I'm not sure "SHN_XINDEX" in this context makes sense, since that isn't the thing that's doing the referring. How about simply "section header string table index <N> does not exist"?

tools/llvm-readobj/ELFDumper.cpp

3021 ↗

(On Diff #209788)

Same comment applies here.

[grimar]

In D64714#1585186, @jhenderson wrote:

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

We shouldn't do this in ELF.h probably, since tools are adding it on their side. e.g. in Invalid.test I am actually testing the file name printed by obj2yaml.
And for example LLD expects that adds a error location (including file names) on it's side too.

Speaking about llvm-readobj/llvm-readelf, I do not mind, going to update the code in ELFDumper.cpp to add a file name.

Aside from that, do we have a way yet of testing the e_shstrndx == SHN_XINDEX case yet for this error? I think that deserves its own test case once we do, so probably should have a TODO note somewhere around the relevant bits of code.

I believe obj2yaml/yaml2obj tool does not support the SHN_XINDEX (https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L901) yet. I'll add such comments.

[jhenderson]

In D64714#1585224, @grimar wrote:

In D64714#1585186, @jhenderson wrote:

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

We shouldn't do this in ELF.h probably, since tools are adding it on their side. e.g. in Invalid.test I am actually testing the file name printed by obj2yaml.
And for example LLD expects that adds a error location (including file names) on it's side too.

Speaking about llvm-readobj/llvm-readelf, I do not mind, going to update the code in ELFDumper.cpp to add a file name.

Sounds good to me.

[grimar]

• Addressed review comments.

[jhenderson]

include/llvm/Object/ELF.h

469 ↗

(On Diff #209846)

It's not SHN_XINDEX that's broken (it can't be - "SHN_XINDEX" is just an enum value), it's the sh_link of the section with index 0.

[grimar]

• Addressed review comments.

include/llvm/Object/ELF.h

469 ↗

(On Diff #209846)

Right. Fixed.

[jhenderson]

LGTM.

llvm/trunk/include/llvm/Object/ELF.h

 Expected<StringRef>
 ELFFile<ELFT>::getSectionStringTable(Elf_Shdr_Range Sections) const {
   uint32_t Index = getHeader()->e_shstrndx;
   if (Index == ELF::SHN_XINDEX)
     Index = Sections[0].sh_link;
 
   if (!Index) // no section string table.
     return "";
+  // TODO: Test a case when the sh_link of the section with index 0 is broken.
   if (Index >= Sections.size())
-    // TODO: this error is untested.
-    return createError("invalid section index");
+    return createError("section header string table index " + Twine(Index) +
+                       " does not exist");
   return getStringTable(&Sections[Index]);
 }
 
 template <class ELFT> ELFFile<ELFT>::ELFFile(StringRef Object) : Buf(Object) {}
 
 template <class ELFT>
 Expected<ELFFile<ELFT>> ELFFile<ELFT>::create(StringRef Object) {
   if (sizeof(Elf_Ehdr) > Object.size())

llvm/trunk/test/Object/invalid.test

- Name: .dynamic
     Entries:
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
   - Type:     PT_DYNAMIC
     FileSize: 0xffff0000
     Sections:
       - Section: .dynamic
+
+# RUN: yaml2obj --docnum=25 %s -o %t25
+# RUN: not obj2yaml 2>&1 %t25 | FileCheck %s -DFILE=%t25 --check-prefix=INVALID-SHSTRNDX
+
+# INVALID-SHSTRNDX: Error reading file: [[FILE]]: section header string table index 255 does not exist
+
+--- !ELF
+FileHeader:
+  Class:    ELFCLASS64
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+  Machine:  EM_X86_64
+  SHStrNdx: 0xFF
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS

llvm/trunk/test/tools/llvm-readobj/elf-invalid-shstrndx.test

+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-readelf --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=GNU
+# RUN: not llvm-readobj --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=LLVM
+
+# GNU:      ELF Header:
+# GNU:        Section header string table index: 255
+# GNU-NEXT:   There are 4 section headers, starting at offset 0x40:
+# GNU:      Section Headers:
+# GNU-NEXT:  [Nr] Name
+# GNU-EMPTY:
+# GNU-NEXT:  error: '[[FILE]]': section header string table index 255 does not exist
+
+# LLVM:      ElfHeader {
+# LLVM:        StringTableSectionIndex: 255
+# LLVM-NEXT: }
+# LLVM-NEXT: Sections [
+# LLVM-EMPTY:
+# LLVM-NEXT: error: '[[FILE]]': section header string table index 255 does not exist
+
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+  SHStrNdx:  0xFF

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

public:
 
   void printCGProfile() override;
   void printAddrsig() override;
 
   void printNotes() override;
 
   void printELFLinkerOptions() override;
 
+  const object::ELFObjectFile<ELFT> *getElfObject() const { return ObjF; };
+
 private:
   std::unique_ptr<DumpStyle<ELFT>> ELFDumperStyle;
 
   TYPEDEF_ELF_TYPES(ELFT)
 
   DynRegionInfo checkDRI(DynRegionInfo DRI) {
     const ELFFile<ELFT> *Obj = ObjF->getELFFile();
     if (DRI.Addr < Obj->base() ||
static std::string getSectionTypeString(unsigned Arch, unsigned Type) {
   default:
     return getSectionTypeOffsetString(Type);
   }
   return "";
 }
 
 template <class ELFT>
 static StringRef getSectionName(const typename ELFT::Shdr &Sec,
-                                const ELFFile<ELFT> &Obj,
+                                const ELFObjectFile<ELFT> &ElfObj,
                                 ArrayRef<typename ELFT::Shdr> Sections) {
+  const ELFFile<ELFT> &Obj = *ElfObj.getELFFile();
   uint32_t Index = Obj.getHeader()->e_shstrndx;
   if (Index == ELF::SHN_XINDEX)
     Index = Sections[0].sh_link;
   if (!Index) // no section string table.
     return "";
+  // TODO: Test a case when the sh_link of the section with index 0 is broken.
   if (Index >= Sections.size())
-    reportError("invalid section index");
+    reportError(ElfObj.getFileName(),
+                createError("section header string table index " +
+                            Twine(Index) + " does not exist"));
   StringRef Data = toStringRef(unwrapOrError(
       Obj.template getSectionContentsAsArray<uint8_t>(&Sections[Index])));
   return unwrapOrError(Obj.getSectionName(&Sec, Data));
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   unsigned Bias = ELFT::Is64Bits ? 0 : 8;
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(Obj->sections());
   OS << "There are " << to_string(Sections.size())
      << " section headers, starting at offset "
      << "0x" << to_hexString(Obj->getHeader()->e_shoff, false) << ":\n\n";
   OS << "Section Headers:\n";
   Field Fields[11] = {
       {"[Nr]", 2},        {"Name", 7},        {"Type", 25},
       {"Address", 41},    {"Off", 58 - Bias}, {"Size", 65 - Bias},
       {"ES", 72 - Bias},  {"Flg", 75 - Bias}, {"Lk", 79 - Bias},
       {"Inf", 82 - Bias}, {"Al", 86 - Bias}};
   for (auto &F : Fields)
     printField(F);
   OS << "\n";
 
+  const ELFObjectFile<ELFT> *ElfObj = this->dumper()->getElfObject();
   size_t SectionIndex = 0;
   for (const Elf_Shdr &Sec : Sections) {
     Fields[0].Str = to_string(SectionIndex);
-    Fields[1].Str = getSectionName(Sec, *Obj, Sections);
+    Fields[1].Str = getSectionName(Sec, *ElfObj, Sections);
     Fields[2].Str =
         getSectionTypeString(Obj->getHeader()->e_machine, Sec.sh_type);
     Fields[3].Str =
         to_string(format_hex_no_prefix(Sec.sh_addr, ELFT::Is64Bits ? 16 : 8));
     Fields[4].Str = to_string(format_hex_no_prefix(Sec.sh_offset, 6));
     Fields[5].Str = to_string(format_hex_no_prefix(Sec.sh_size, 6));
     Fields[6].Str = to_string(format_hex_no_prefix(Sec.sh_entsize, 2));
     Fields[7].Str = getGNUFlags(Sec.sh_flags);

 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   ListScope SectionsD(W, "Sections");
 
   int SectionIndex = -1;
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(Obj->sections());
+  const ELFObjectFile<ELFT> *ElfObj = this->dumper()->getElfObject();
   for (const Elf_Shdr &Sec : Sections) {
-    StringRef Name = getSectionName(Sec, *Obj, Sections);
+    StringRef Name = getSectionName(Sec, *ElfObj, Sections);
     DictScope SectionD(W, "Section");
     W.printNumber("Index", ++SectionIndex);
     W.printNumber("Name", Name, Sec.sh_name);
     W.printHex(
         "Type",
         object::getELFSectionTypeName(Obj->getHeader()->e_machine, Sec.sh_type),
         Sec.sh_type);
     std::vector<EnumEntry<unsigned>> SectionFlags(std::begin(ElfSectionFlags),

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 reportError(StringRef Input, Error Err); 
   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);
   }

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

namespace opts {
 
   cl::extrahelp
       HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 } // namespace opts
 
 namespace llvm {
 
 LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
+  fouts().flush();
   errs() << "\n";
   WithColor::error(errs()) << Msg << "\n";
   exit(1);
 }
 
+void reportError(StringRef Input, Error Err) {
+  if (Input == "-")
+    Input = "<stdin>";
+  error(createFileError(Input, std::move(Err)));
+}
+
 void reportWarning(Twine Msg) {
   fouts().flush();
   errs() << "\n";
   WithColor::warning(errs()) << Msg << "\n";
 }
 
 void warn(Error Err) {
   handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {

 void error(std::error_code EC) {
   if (!EC)
     return;
   reportError(EC.message());
 }
 
 } // namespace llvm
 
-static void reportError(StringRef Input, Error Err) {
-  if (Input == "-")
-    Input = "<stdin>";
-  error(createFileError(Input, std::move(Err)));
-}
-
 static void reportError(StringRef Input, std::error_code EC) {
   reportError(Input, errorCodeToError(EC));
 }
 
 static bool isMipsArch(unsigned Arch) {
   switch (Arch) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel: