[llvm-readobj/libObject] - Allow dumping objects that has a broken SHT_SYMTAB_SHNDX section.

[grimar]

Summary

Currently it is impossible to create an instance of ELFObjectFile when the
SHT_SYMTAB_SHNDX can't be read. We error out when fail to parse the
SHT_SYMTAB_SHNDX section in the factory method.

This change delays reading of the SHT_SYMTAB_SHNDX section entries,
with it llvm-readobj is now able to work with such inputs.

[MaskRay]

Currently it is impossible to create an instance of ELFObjectFile when the SHT_SYMTAB_SHNDX can't be read.

.. it errors ... when failed to parse SHT_SYMTAB_SHNDX

It happens because ELFObjectFile tries to read its entries too early, in the constructor.

ELFObjectFile::create is not a constructor. It is a factory function.

llvm/include/llvm/Object/ELFObjectFile.h

699

Is it because yaml2obj cannot emit a malformed SHT_SYMTAB_SHNDX?

llvm/unittests/Object/ELFObjectFileTest.cpp

303

Seems misaligned

[grimar]

• Addressed review comments.

llvm/include/llvm/Object/ELFObjectFile.h

699

No, it is because I think that testing of this place is a bit unrelated to what this patch does.
The idea of the patch is to allow to create an instance of ELFObjectFile when the
SHT_SYMTAB_SHNDX can't be read. I had to update this place because of API change,
but hopefully it doesn't mean I need to add a test for each place that can now report an error right now
(given that there are few others "Test this error" TODOs in this file).

Note, that this method (getSymbolSection) is very specific: it is only invoked by llvm-readobj by following 3 tests:

LLVM :: tools/llvm-readobj/ELF/AArch64/dwarf-cfi.s
LLVM :: tools/llvm-readobj/ELF/ARM/dwarf-cfi.s
LLVM :: tools/llvm-readobj/ELF/stack-sizes.test

First two are using it because lib/DebugInfo/DWARF (DWARFObjInMemory) calls it.
Stack sizes implementation also uses ELFObjectFile<ELFT>::getSymbolSection(DataRefImpl Symb) API,
though it should probably use ELFFile<ELFT> instead (see D87362).

[jhenderson]

llvm/include/llvm/Object/ELFObjectFile.h

697

Debugging code needs removing?

llvm/test/tools/llvm-readobj/ELF/ARM/dwarf-cfi.s

1–2 ↗

(On Diff #299289)

Looks like you don't need to do this as part of this change, since this file is otherwise unaffected?

llvm/test/tools/llvm-readobj/ELF/symbol-shndx.test

258

265

Presumably not part of this patch, since I can't see the error message being reported anywhere in the changes, but this should be "the associated symbol table".

llvm/unittests/Object/ELFObjectFileTest.cpp

314

The test name here doesn't correspond with the behaviour being tested.

[grimar]

llvm/include/llvm/Object/ELFObjectFile.h

697

Oops.

llvm/test/tools/llvm-readobj/ELF/ARM/dwarf-cfi.s

1–2 ↗

(On Diff #299289)

Right. I think I forgot to revert changes I did for debugging..

[grimar]

• Addressed review comments.

llvm/test/tools/llvm-readobj/ELF/symbol-shndx.test

265

Yeah. It comes from ELFFile<ELFT>::getSHNDXTable. I'll fix.

[jhenderson]

Looks good, (I'm ambivalent on the testing discussion, and will let you and @MaskRay come to a conclusion).

[grimar]

@MaskRay, are you OK with the current version?

[grimar]

Ping, @MaskRay?

[grimar]

Ping @MaskRay?
If there will be no objections, I am going to commit this in my tomorrow.

llvm/include/llvm/Object/ELFObjectFile.h

public:
 
   SectionRef toSectionRef(const Elf_Shdr *Sec) const {
     return SectionRef(toDRI(Sec), this);
   }
 
 private:
   ELFObjectFile(MemoryBufferRef Object, ELFFile<ELFT> EF,
                 const Elf_Shdr *DotDynSymSec, const Elf_Shdr *DotSymtabSec,
-                ArrayRef<Elf_Word> ShndxTable);
+                const Elf_Shdr *DotSymtabShndxSec);
 
 protected:
   ELFFile<ELFT> EF;
 
   const Elf_Shdr *DotDynSymSec = nullptr; // Dynamic symbol table section.
   const Elf_Shdr *DotSymtabSec = nullptr; // Symbol table section.
-  ArrayRef<Elf_Word> ShndxTable;
+  const Elf_Shdr *DotSymtabShndxSec = nullptr; // SHT_SYMTAB_SHNDX section.
 
   void moveSymbolNext(DataRefImpl &Symb) const override;
   Expected<StringRef> getSymbolName(DataRefImpl Symb) const override;
   Expected<uint64_t> getSymbolAddress(DataRefImpl Symb) const override;
   uint64_t getSymbolValueImpl(DataRefImpl Symb) const override;
   uint32_t getSymbolAlignment(DataRefImpl Symb) const override;
   uint64_t getCommonSymbolSizeImpl(DataRefImpl Symb) const override;
   Expected<uint32_t> getSymbolFlags(DataRefImpl Symb) const override;
case ELF::SHN_ABS:
     return Result;
   }
 
   auto SymTabOrErr = EF.getSection(Symb.d.a);
   if (!SymTabOrErr)
     return SymTabOrErr.takeError();
 
   if (EF.getHeader().e_type == ELF::ET_REL) {
+    ArrayRef<Elf_Word> ShndxTable;
+    if (DotSymtabShndxSec) {
+      // TODO: Test this error.
+      if (Expected<ArrayRef<Elf_Word>> ShndxTableOrErr =
+              EF.getSHNDXTable(*DotSymtabShndxSec))
+        ShndxTable = *ShndxTableOrErr;
+      else
+        return ShndxTableOrErr.takeError();
+    }
+
     Expected<const Elf_Shdr *> SectionOrErr =
         EF.getSection(*ESym, *SymTabOrErr, ShndxTable);
     if (!SectionOrErr)
       return SectionOrErr.takeError();
     const Elf_Shdr *Section = *SectionOrErr;
     if (Section)
       Result += Section->sh_addr;
   }
Expected<uint32_t> ELFObjectFile<ELFT>::getSymbolFlags(DataRefImpl Sym) const {
 
   return Result;
 }
 
 template <class ELFT>
 Expected<section_iterator>
 ELFObjectFile<ELFT>::getSymbolSection(const Elf_Sym *ESym,
                                       const Elf_Shdr *SymTab) const {
+  ArrayRef<Elf_Word> ShndxTable;
+  if (DotSymtabShndxSec) {
+    // TODO: Test this error.
+    Expected<ArrayRef<Elf_Word>> ShndxTableOrErr =
+        EF.getSHNDXTable(*DotSymtabShndxSec);
+    if (!ShndxTableOrErr)
+      return ShndxTableOrErr.takeError();
+    ShndxTable = *ShndxTableOrErr;
+  }
+
   auto ESecOrErr = EF.getSection(*ESym, SymTab, ShndxTable);
   if (!ESecOrErr)
     return ESecOrErr.takeError();
 
   const Elf_Shdr *ESec = *ESecOrErr;
   if (!ESec)
     return section_end();
 
ELFObjectFile<ELFT>::create(MemoryBufferRef Object) {
   auto EF = std::move(*EFOrErr);
 
   auto SectionsOrErr = EF.sections();
   if (!SectionsOrErr)
     return SectionsOrErr.takeError();
 
   const Elf_Shdr *DotDynSymSec = nullptr;
   const Elf_Shdr *DotSymtabSec = nullptr;
-  ArrayRef<Elf_Word> ShndxTable;
+  const Elf_Shdr *DotSymtabShndxSec = nullptr;
   for (const Elf_Shdr &Sec : *SectionsOrErr) {
     switch (Sec.sh_type) {
     case ELF::SHT_DYNSYM: {
       if (!DotDynSymSec)
         DotDynSymSec = &Sec;
       break;
     }
     case ELF::SHT_SYMTAB: {
       if (!DotSymtabSec)
         DotSymtabSec = &Sec;
       break;
     }
     case ELF::SHT_SYMTAB_SHNDX: {
-      auto TableOrErr = EF.getSHNDXTable(Sec);
-      if (!TableOrErr)
-        return TableOrErr.takeError();
-      ShndxTable = *TableOrErr;
+      if (!DotSymtabShndxSec)
+        DotSymtabShndxSec = &Sec;
       break;
     }
     }
   }
   return ELFObjectFile<ELFT>(Object, EF, DotDynSymSec, DotSymtabSec,
-                             ShndxTable);
+                             DotSymtabShndxSec);
 }
 
 template <class ELFT>
 ELFObjectFile<ELFT>::ELFObjectFile(MemoryBufferRef Object, ELFFile<ELFT> EF,
                                    const Elf_Shdr *DotDynSymSec,
                                    const Elf_Shdr *DotSymtabSec,
-                                   ArrayRef<Elf_Word> ShndxTable)
+                                   const Elf_Shdr *DotSymtabShndx)
     : ELFObjectFileBase(
           getELFType(ELFT::TargetEndianness == support::little, ELFT::Is64Bits),
           Object),
       EF(EF), DotDynSymSec(DotDynSymSec), DotSymtabSec(DotSymtabSec),
-      ShndxTable(ShndxTable) {}
+      DotSymtabShndxSec(DotSymtabShndx) {}
 
 template <class ELFT>
 ELFObjectFile<ELFT>::ELFObjectFile(ELFObjectFile<ELFT> &&Other)
     : ELFObjectFile(Other.Data, Other.EF, Other.DotDynSymSec,
-                    Other.DotSymtabSec, Other.ShndxTable) {}
+                    Other.DotSymtabSec, Other.DotSymtabShndxSec) {}
 
 template <class ELFT>
 basic_symbol_iterator ELFObjectFile<ELFT>::symbol_begin() const {
   DataRefImpl Sym =
       toDRI(DotSymtabSec,
             DotSymtabSec && DotSymtabSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);
   return basic_symbol_iterator(SymbolRef(Sym, this));
 }

llvm/test/Object/invalid.test

- Name: .symtab
     Type: SHT_SYMTAB
     Size: 1
 
 ## Test that llvm-readobj reports an error if SHT_SYMTAB_SHNDX section has
 ## invalid sh_size which should be:
 ## sizeof(.symtab_shndx) = (sizeof(.symtab) / entsize(.symtab)) * entsize(.symtab_shndx)
 
 # RUN: yaml2obj %s --docnum=11 -o %t11
-# RUN: not llvm-readobj --symbols %t11 2>&1 | FileCheck --check-prefix=INVALID-XINDEX-SIZE %s
+# RUN: llvm-readobj --symbols %t11 2>&1 | FileCheck --check-prefix=INVALID-XINDEX-SIZE %s
 
-# INVALID-XINDEX-SIZE: error: {{.*}}: SHT_SYMTAB_SHNDX has 2 entries, but the symbol table associated has 1
+# INVALID-XINDEX-SIZE: warning: {{.*}}: SHT_SYMTAB_SHNDX has 2 entries, but the symbol table associated has 1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:

llvm/test/tools/llvm-readobj/ELF/symbol-shndx.test

FileHeader:
   Class: ELFCLASS32
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Symbols:
   - Name:  no_shndx
     Index: SHN_XINDEX
   - Name:  no_shndx2
     Index: SHN_XINDEX
+
+## Check we can dump symbols even when the number of entries in the
+## SHT_SYMTAB_SHNDX section doesn't match the number of symbols in the symbol table.
+
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-readelf --symbols %t4 2>&1 | FileCheck %s -DFILE=%t4 --check-prefix=SHNDX-ERR-GNU
+# RUN: llvm-readobj --symbols %t4 2>&1 | FileCheck %s -DFILE=%t4 --check-prefix=SHNDX-ERR-LLVM
+
+#       SHNDX-ERR-GNU: warning: '[[FILE]]': SHT_SYMTAB_SHNDX has 3 entries, but the symbol table associated has 2
+# SHNDX-ERR-GNU-EMPTY:
+#  SHNDX-ERR-GNU-NEXT: Symbol table '.symtab' contains 2 entries:
+#  SHNDX-ERR-GNU-NEXT:    Num:    Value          Size Type    Bind   Vis       Ndx Name
+#  SHNDX-ERR-GNU-NEXT:      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND
+#  SHNDX-ERR-GNU-NEXT: warning: '[[FILE]]': extended symbol index (1) is past the end of the SHT_SYMTAB_SHNDX section of size 0
+#  SHNDX-ERR-GNU-NEXT:      1: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   RSV[0xffff]
+# SHNDX-ERR-GNU-EMPTY:
+#   SHNDX-ERR-GNU-NOT:{{.}}
+
+# SHNDX-ERR-LLVM: warning: '[[FILE]]': SHT_SYMTAB_SHNDX has 3 entries, but the symbol table associated has 2
+# SHNDX-ERR-LLVM:      Format: elf64-unknown
+# SHNDX-ERR-LLVM-NEXT: Arch: unknown
+# SHNDX-ERR-LLVM-NEXT: AddressSize: 64bit
+# SHNDX-ERR-LLVM-NEXT: LoadName: <Not found>
+# SHNDX-ERR-LLVM-NEXT: Symbols [
+# SHNDX-ERR-LLVM-NEXT:   Symbol {
+# SHNDX-ERR-LLVM-NEXT:     Name:  (0)
+# SHNDX-ERR-LLVM-NEXT:     Value: 0x0
+# SHNDX-ERR-LLVM-NEXT:     Size: 0
+# SHNDX-ERR-LLVM-NEXT:     Binding: Local (0x0)
+# SHNDX-ERR-LLVM-NEXT:     Type: None (0x0)
+# SHNDX-ERR-LLVM-NEXT:     Other: 0
+# SHNDX-ERR-LLVM-NEXT:     Section: Undefined (0x0)
+# SHNDX-ERR-LLVM-NEXT:   }
+# SHNDX-ERR-LLVM-NEXT:   Symbol {
+# SHNDX-ERR-LLVM-NEXT:     Name:  (0)
+# SHNDX-ERR-LLVM-NEXT:     Value: 0x0
+# SHNDX-ERR-LLVM-NEXT:     Size: 0
+# SHNDX-ERR-LLVM-NEXT:     Binding: Local (0x0)
+# SHNDX-ERR-LLVM-NEXT:     Type: None (0x0)
+# SHNDX-ERR-LLVM-NEXT:     Other: 0
+# SHNDX-ERR-LLVM-NEXT: warning: '[[FILE]]': extended symbol index (1) is past the end of the SHT_SYMTAB_SHNDX section of size 0
+# SHNDX-ERR-LLVM-NEXT:     Section: Reserved (0xFFFF)
+# SHNDX-ERR-LLVM-NEXT:   }
+# SHNDX-ERR-LLVM-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL
+Sections:
+  - Name:    .symtab_shndx
+    Type:    SHT_SYMTAB_SHNDX
+    Entries: [ 0, 1, 2 ]
+    Link:    .symtab
+Symbols:
+  - Index: SHN_XINDEX

llvm/test/tools/obj2yaml/ELF/sht-symtab-shndx.yaml

 Symbols: []
 
 ## Check that yaml2obj can't dump the object if SHT_SYMTAB_SHNDX is
 ## not associated with a SHT_SYMTAB section (this case is illegal).
 
 # RUN: yaml2obj --docnum=6 %s -o %t6
 # RUN: not obj2yaml %t6 2>&1 | FileCheck %s -DFILE=%t6 --check-prefix=CASE6
 
-# CASE6: Error reading file: [[FILE]]: SHT_SYMTAB_SHNDX section is linked with SHT_PROGBITS section (expected SHT_SYMTAB/SHT_DYNSYM)
+# CASE6: Error reading file: [[FILE]]: only SHT_SYMTAB_SHNDX associated with SHT_SYMTAB are supported
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Sections:
   - Name:    .symtab_shndx

llvm/test/tools/yaml2obj/ELF/sht-symtab-shndx.yaml

FileHeader:
   Type:  ET_REL
 Symbols:
   - Type:  STT_SECTION
     Index: 65536
 
 ## Check we can set a custom sh_entsize for SHT_SYMTAB_SHNDX section.
 
 # RUN: yaml2obj --docnum=5 %s -o %t5
-# RUN: not llvm-readelf -S 2>&1 %t5 | FileCheck %s -DFILE=%t5 --check-prefix=CASE5
+# RUN: llvm-readelf -S 2>&1 %t5 | FileCheck %s -DFILE=%t5 --check-prefix=CASE5
 
-# CASE5: error: '[[FILE]]': section [index 1] has an invalid sh_entsize: 2
+# CASE5: warning: '[[FILE]]': section [index 1] has an invalid sh_entsize: 2
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Sections:
   - Name:    .symtab_shndx

llvm/tools/llvm-readobj/ELFDumper.cpp

case ELF::SHT_DYNSYM:
         } else {
           reportUniqueWarning(createError(
               "unable to read dynamic symbols from " + describe(Sec) + ": " +
               toString(RegOrErr.takeError())));
         }
       }
       break;
     case ELF::SHT_SYMTAB_SHNDX:
-      ShndxTable = unwrapOrError(ObjF.getFileName(), Obj.getSHNDXTable(Sec));
+      if (Expected<ArrayRef<Elf_Word>> ShndxTableOrErr = Obj.getSHNDXTable(Sec))
+        ShndxTable = *ShndxTableOrErr;
+      else
+        this->reportUniqueWarning(ShndxTableOrErr.takeError());
       break;
     case ELF::SHT_GNU_versym:
       if (!SymbolVersionSection)
         SymbolVersionSection = &Sec;
       break;
     case ELF::SHT_GNU_verdef:
       if (!SymbolVersionDefSection)
         SymbolVersionDefSection = &Sec;

llvm/unittests/Object/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BinaryFormat
   Object
+  ObjectYAML
   )
 
 add_llvm_unittest(ObjectTests
   ArchiveTest.cpp
   ELFObjectFileTest.cpp
   ELFTypesTest.cpp
   ELFTest.cpp
   MinidumpTest.cpp
   ObjectFileTest.cpp
   SymbolSizeTest.cpp
   SymbolicFileTest.cpp
   XCOFFObjectFileTest.cpp
   )
 
 target_link_libraries(ObjectTests PRIVATE LLVMTestingSupport)

llvm/unittests/Object/ELFObjectFileTest.cpp

 //===- ELFObjectFileTest.cpp - Tests for ELFObjectFile --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
 namespace {
 

 TEST(ELFObjectFileTest, MachineTestForCSKY) {
   std::array<StringRef, 4> Formats = {"elf32-csky", "elf32-csky",
                                       "elf64-unknown", "elf64-unknown"};
   size_t I = 0;
   for (const DataForTest &D : generateData(ELF::EM_CSKY))
     checkFormatAndArch(D, Formats[I++], Triple::csky);
 }
 
-
-
 // ELF relative relocation type test.
 TEST(ELFObjectFileTest, RelativeRelocationTypeTest) {
   EXPECT_EQ(ELF::R_CKCORE_RELATIVE, getELFRelativeRelocationType(ELF::EM_CSKY));
 }
+
+template <class ELFT>
+static Expected<ELFObjectFile<ELFT>> toBinary(SmallVectorImpl<char> &Storage,
+                                              StringRef Yaml) {
+  raw_svector_ostream OS(Storage);
+  yaml::Input YIn(Yaml);
+  if (!yaml::convertYAML(YIn, OS, [](const Twine &Msg) {}))
+    return createStringError(std::errc::invalid_argument,
+                             "unable to convert YAML");
+  return ELFObjectFile<ELFT>::create(MemoryBufferRef(OS.str(), "dummyELF"));
+}
+
+// Check we are able to create an ELFObjectFile even when the content of the
+// SHT_SYMTAB_SHNDX section can't be read properly.
+TEST(ELFObjectFileTest, InvalidSymtabShndxTest) {
+  SmallString<0> Storage;
+  Expected<ELFObjectFile<ELF64LE>> ExpectedFile = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL
+Sections:
+  - Name:    .symtab_shndx
+    Type:    SHT_SYMTAB_SHNDX
+    Entries: [ 0 ]
+    ShSize: 0xFFFFFFFF
+)");
+
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+}