[Object] object::ELFObjectFile::symbol_begin(): skip symbol index 0

[MaskRay]

Summary

For clients iterating the symbol table, none expects to handle index 0
(STN_UNDEF). Skip it to improve consistency with other binary formats.
Clients that need STN_UNDEF (e.g. lld) can use
getSectionContentsAsArray().

[jhenderson]

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

[MaskRay]

In D62296#1514030, @jhenderson wrote:

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

I've checked the call sites:

In RuntimeDyld, if (Flags & SymbolRef::SF_Undefined) continue; filters out index 0.
In llvm-objdump, the filtering code is removed in this patch.
In lib/Object/SymbolSize.cpp, I think the code doesn't notice index 0 is included... but it doesn't seem to matter.
In llvm-nm.cpp, I'll leave the test to @chrisjackson in D62148

[chrisjackson]

In D62296#1514082, @MaskRay wrote:

In D62296#1514030, @jhenderson wrote:

Have you made sure that there aren't any uses which would result in an undesired change in behaviour because of this? I feel like it's something that people wouldn't expect to change, and therefore wouldn't test explicitly necessarily.

There should also be a test case somewhere (not that I have a good suggestion for where).

I've checked the call sites:

In RuntimeDyld, if (Flags & SymbolRef::SF_Undefined) continue; filters out index 0.
In llvm-objdump, the filtering code is removed in this patch.
In lib/Object/SymbolSize.cpp, I think the code doesn't notice index 0 is included... but it doesn't seem to matter.
In llvm-nm.cpp, I'll leave the test to @chrisjackson in D62148

I have included the test as part of D62955.

llvm/trunk/include/llvm/Object/ELFObjectFile.h

 
 template <class ELFT>
 ELFObjectFile<ELFT>::ELFObjectFile(ELFObjectFile<ELFT> &&Other)
     : ELFObjectFile(Other.Data, Other.EF, Other.DotDynSymSec,
                     Other.DotSymtabSec, Other.ShndxTable) {}
 
 template <class ELFT>
 basic_symbol_iterator ELFObjectFile<ELFT>::symbol_begin() const {
-  DataRefImpl Sym = toDRI(DotSymtabSec, 0);
+  DataRefImpl Sym =
+      toDRI(DotSymtabSec,
+            DotSymtabSec && DotSymtabSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);
   return basic_symbol_iterator(SymbolRef(Sym, this));
 }
 
 template <class ELFT>
 basic_symbol_iterator ELFObjectFile<ELFT>::symbol_end() const {
   const Elf_Shdr *SymTab = DotSymtabSec;
   if (!SymTab)
     return symbol_begin();

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

void printSymbolTable(const ObjectFile *O, StringRef ArchiveName,
 
   if (const COFFObjectFile *Coff = dyn_cast<const COFFObjectFile>(O)) {
     printCOFFSymbolTable(Coff);
     return;
   }
 
   const StringRef FileName = O->getFileName();
   for (auto I = O->symbol_begin(), E = O->symbol_end(); I != E; ++I) {
-    // Skip printing the special zero symbol when dumping an ELF file.
-    // This makes the output consistent with the GNU objdump.
-    if (I == O->symbol_begin() && isa<ELFObjectFileBase>(O))
-      continue;
-
     const SymbolRef &Symbol = *I;
     uint64_t Address = unwrapOrError(Symbol.getAddress(), ArchiveName, FileName,
                                      ArchitectureName);
     if ((Address < StartAddress) || (Address > StopAddress))
       continue;
     SymbolRef::Type Type = unwrapOrError(Symbol.getType(), ArchiveName,
                                          FileName, ArchitectureName);
     uint32_t Flags = Symbol.getFlags();