[llvm-nm/objdump/size] Add tests for dumping symbol tables with invalid sh_size.

[Higuoxing]

Summary

This change adds tests for llvm-nm, llvm-objdump and llvm-size when dumping symbol tables with invalid sh_size (sh_size % sizeof(Elf_Sym) != 0).

[jhenderson]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

1

will emit -> emits

(same in other tests)

llvm/test/tools/llvm-size/invalid-symbol-table-size.test

5

I think you need testing for both the sysv and berkeley output formats, as you've handled the error in two different places.

[grimar]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

In this and other test cases you have 2 sections: .symtab and .dynsym, but only one section (with index 2)
is reported in error messages.

[Higuoxing]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

Yes, because there are malformed .symtab and .dynsym symbol tables in one ELF file. And getSymbolFlags() checks .symtab first. See ELFObjectFile.h.

if (Expected<typename ELFT::SymRange> SymbolsOrErr =
        EF.symbols(DotSymtabSec)) {
  ...
} else
  // TODO: Test this error.
  return SymbolsOrErr.takeError();

if (Expected<typename ELFT::SymRange> SymbolsOrErr =
        EF.symbols(DotDynSymSec)) {
  ...
} else
  // TODO: Test this error.
  return SymbolsOrErr.takeError();

I think it's a good point, and it's helpful to report errors on our interested sections.

---

Currently, I have two approaches in mind.

1. We could add one more argument IsDynamic to getSymbolFlags(DataRefImpl Symb, bool IsDynamic=false). So that we can skip first checker if we've already known the symbol is a dynamic symbol. (Possible)

2. Change the return type of symbol iterator API from symbol_iterator to Expected<symbol_iterator>, so that we could get error reports earlier. (I didn't give it a try, and it seems involving many changes.)

[grimar]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

Why not to change the test so that you have a one broken test at once?

Case 1: Broken .symtab. Valid .dynsym.
Case 2: Valid .symtab. Broken .dynsym.

12

• a one broken section

[Higuoxing]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

I think change test cases partly resolve this problem. I mean, if we one broken .symtab, and a good .dynsym section, consider the following situation.

# broken .symtab prevents us dumping .dynsym
$ llvm-nm --dynamic broken-symtab.elf
$ llvm-objdump --dynamic-syms broken-symtab.elf

[grimar]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

You can have

Case 3: Both .symtab and .dynsym are broken. The same behavior as in Case 1.

[Higuoxing]

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

12

Ok, thanks a lot.

[Higuoxing]

Addressed comments.

• Test .symtab and .dynsym separately.
• Add tests in llvm-size for different output format (sysv and berkeley).

[jhenderson]

LGTM! @grimar is away this week, I believe, so I don't think you need to wait for him.

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test

+## This test ensures llvm-nm will emit a helpful error message when dumping a symbol table
+## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
+
+# RUN: yaml2obj -DBITS=32 -DSIZE=33 %s -o %t.32-bit.o
+# RUN: not llvm-nm %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 %s
+# RUN: yaml2obj -DBITS=64 -DSIZE=49 %s -o %t.64-bit.o
+# RUN: not llvm-nm %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 %s
+
+# RUN: not llvm-nm --dynamic %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 %s
+# RUN: not llvm-nm --dynamic %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 %s
+
+# CHECK: error: {{.*}} section [index 2] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS[[BITS]]
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_386
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Content: "00"
+  - Name:    .symtab
+    Type:    SHT_SYMTAB
+    Size:    [[SIZE]]
+  - Name:    .dynsym
+    Type:    SHT_DYNSYM
+    Size:    [[SIZE]]

llvm/test/tools/llvm-objdump/invalid-symbol-table-size.test

+## This test ensures llvm-objdump will emit a helpful error message when dumping a symbol table
+## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
+
+# RUN: yaml2obj -DBITS=32 -DSIZE=33 %s -o %t.32-bit.o
+# RUN: not llvm-objdump --syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 %s
+# RUN: yaml2obj -DBITS=64 -DSIZE=49 %s -o %t.64-bit.o
+# RUN: not llvm-objdump --syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 %s
+
+# RUN: not llvm-objdump --dynamic-syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 %s
+# RUN: not llvm-objdump --dynamic-syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 %s
+
+# CHECK: error: {{.*}} section [index 2] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS[[BITS]]
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_386
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Content: "00"
+  - Name:    .symtab
+    Type:    SHT_SYMTAB
+    Size:    [[SIZE]]
+  - Name:    .dynsym
+    Type:    SHT_DYNSYM
+    Size:    [[SIZE]]

llvm/test/tools/llvm-size/invalid-symbol-table-size.test

+## This test ensures llvm-size will emit a helpful error message when dumping a symbol table
+## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
+
+# RUN: yaml2obj -DBITS=32 -DSIZE=33 %s -o %t.32-bit.o
+# RUN: not llvm-size --common %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 %s
+# RUN: yaml2obj -DBITS=64 -DSIZE=49 %s -o %t.64-bit.o
+# RUN: not llvm-size --common %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 %s
+
+# CHECK: error: '{{.*}}': section [index 1] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS[[BITS]]
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_386
+Sections:
+  - Name:    .symtab
+    Type:    SHT_SYMTAB
+    Size:    [[SIZE]]

llvm/tools/llvm-nm/llvm-nm.cpp

for (const NMSymbol &S : SymbolList) {
     std::string Name = S.Name.str();
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
     if (Demangle) {
       if (Optional<std::string> Opt = demangle(S.Name, MachO))
         Name = *Opt;
     }
     if (S.Sym.getRawDataRefImpl().p) {
       Expected<uint32_t> SymFlagsOrErr = S.Sym.getFlags();
-      if (!SymFlagsOrErr)
-        // TODO: This error should be reported and tested properly.
-        report_fatal_error(SymFlagsOrErr.takeError());
+      if (!SymFlagsOrErr) {
+        error(SymFlagsOrErr.takeError(), Obj.getFileName());
+        return;
+      }
       SymFlags = *SymFlagsOrErr;
     } else
       SymFlags = S.SymFlags;
 
     bool Undefined = SymFlags & SymbolRef::SF_Undefined;
     bool Global = SymFlags & SymbolRef::SF_Global;
     bool Weak = SymFlags & SymbolRef::SF_Weak;
     if ((!Undefined && UndefinedOnly) || (Undefined && DefinedOnly) ||
if (!SegSect.empty() && MachO) {
     Nsect = getNsectForSegSect(MachO);
     // If this section is not in the object file no symbols are printed.
     if (Nsect == 0)
       return;
   }
   if (!(MachO && DyldInfoOnly)) {
     for (BasicSymbolRef Sym : Symbols) {
       Expected<uint32_t> SymFlagsOrErr = Sym.getFlags();
-      if (!SymFlagsOrErr)
-        // TODO: This error should be reported and tested properly.
-        report_fatal_error(SymFlagsOrErr.takeError());
+      if (!SymFlagsOrErr) {
+        error(SymFlagsOrErr.takeError(), Obj.getFileName());
+        return;
+      }
       if (!DebugSyms && (*SymFlagsOrErr & SymbolRef::SF_FormatSpecific))
         continue;
       if (WithoutAliases && (*SymFlagsOrErr & SymbolRef::SF_Indirect))
         continue;
       // If a "-s segname sectname" option was specified and this is a Mach-O
       // file and this section appears in this file, Nsect will be non-zero then
       // see if this symbol is a symbol from that section and if not skip it.
       if (Nsect && Nsect != getNsectInMachO(*MachO, Sym))

llvm/tools/llvm-objdump/llvm-objdump.cpp

void printSymbol(const ObjectFile *O, const SymbolRef &Symbol,
                  StringRef ArchitectureName, bool DumpDynamic) {
   const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(O);
   uint64_t Address = unwrapOrError(Symbol.getAddress(), FileName, ArchiveName,
                                    ArchitectureName);
   if ((Address < StartAddress) || (Address > StopAddress))
     return;
   SymbolRef::Type Type =
       unwrapOrError(Symbol.getType(), FileName, ArchiveName, ArchitectureName);
-  Expected<uint32_t> FlagsOrErr = Symbol.getFlags();
-  if (!FlagsOrErr)
-    // TODO: This error should be reported and tested properly.
-    report_fatal_error(FlagsOrErr.takeError());
-  uint32_t Flags = *FlagsOrErr;
+  uint32_t Flags =
+      unwrapOrError(Symbol.getFlags(), FileName, ArchiveName, ArchitectureName);
 
   // Don't ask a Mach-O STAB symbol for its section unless you know that
   // STAB symbol's section field refers to a valid section index. Otherwise
   // the symbol may error trying to load a section that does not exist.
   bool IsSTAB = false;
   if (MachO) {
     DataRefImpl SymDRI = Symbol.getRawDataRefImpl();
     uint8_t NType =

llvm/tools/llvm-size/llvm-size.cpp

static bool considerForSize(ObjectFile *Obj, SectionRef Section) {
   case ELF::SHT_REL:
   case ELF::SHT_RELA:
     return false;
   }
   return true;
 }
 
 /// Total size of all ELF common symbols
-static uint64_t getCommonSize(ObjectFile *Obj) {
+static Expected<uint64_t> getCommonSize(ObjectFile *Obj) {
   uint64_t TotalCommons = 0;
   for (auto &Sym : Obj->symbols()) {
     auto SymFlagsOrErr = Obj->getSymbolFlags(Sym.getRawDataRefImpl());
     if (!SymFlagsOrErr)
-      // TODO: This error should be reported and tested properly.
-      report_fatal_error(SymFlagsOrErr.takeError());
+      return SymFlagsOrErr.takeError();
     if (*SymFlagsOrErr & SymbolRef::SF_Common)
       TotalCommons += Obj->getCommonSymbolSize(Sym.getRawDataRefImpl());
   }
   return TotalCommons;
 }
 
 /// Print the size of each Mach-O segment and section in @p MachO.
 ///
for (const SectionRef &Section : Obj->sections()) {
       }
 
       uint64_t size = Section.getSize();
       uint64_t addr = Section.getAddress();
       outs() << format(fmt.str().c_str(), name_or_err->str().c_str(), size, addr);
     }
 
     if (ELFCommons) {
-      uint64_t CommonSize = getCommonSize(Obj);
-      total += CommonSize;
+      Expected<uint64_t> CommonSizeOrErr = getCommonSize(Obj);
+      if (!CommonSizeOrErr) {
+        error(CommonSizeOrErr.takeError(), Obj->getFileName());
+        return;
+      }
+      total += *CommonSizeOrErr;
       outs() << format(fmt.str().c_str(), std::string("*COM*").c_str(),
-                       CommonSize, static_cast<uint64_t>(0));
+                       *CommonSizeOrErr, static_cast<uint64_t>(0));
     }
 
     // Print total.
     fmtbuf.clear();
     fmt << "%-" << max_name_len << "s "
         << "%#" << max_size_len << radix_fmt << "\n";
     outs() << format(fmt.str().c_str(), static_cast<const char *>("Total"),
                      total)
for (const SectionRef &Section : Obj->sections()) {
       if (isText)
         total_text += size;
       else if (isData)
         total_data += size;
       else if (isBSS)
         total_bss += size;
     }
 
-    if (ELFCommons)
-      total_bss += getCommonSize(Obj);
+    if (ELFCommons) {
+      Expected<uint64_t> CommonSizeOrErr = getCommonSize(Obj);
+      if (!CommonSizeOrErr) {
+        error(CommonSizeOrErr.takeError(), Obj->getFileName());
+        return;
+      }
+      total_bss += *CommonSizeOrErr;
+    }
 
     total = total_text + total_data + total_bss;
 
     if (TotalSizes) {
       TotalObjectText += total_text;
       TotalObjectData += total_data;
       TotalObjectBss += total_bss;
       TotalObjectTotal += total;