[llvm-objcopy] Add e_machine validity check for reserved section indexes

[jakehehrlich]

Summary

As discussed on llvm-commits it was decided it would be best to check e_machine before declaring that a reserved section index is valid. The only special e_machine value that matters here is EM_HEXAGON. This change adds a special check for EM_HEXAGAON.

[mcgrathr]

tools/llvm-objcopy/Object.cpp

102

This will work as written, but I think a different structure would better reflect what you're really doing.
That is, have a first switch for the common stuff, with no default.
Then do if (Machine == ...) switch ... for each set of machine-specific ones.
Then just return false at the end if no switch matched.
That will be right in the future when another machine has a different set that are actually the same numbers as SHN_HEXAGON_*.

[jakehehrlich]

Very true! I wasn't thinking about the fact that other architectures would reuse the same values.

[mcgrathr]

LGTM

[jhenderson]

Would it be possible to add a test that shows that the special Hexagon types aren't valid for other targets, say x86?

test/tools/llvm-objcopy/common-symbol.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy %t %t2
 # RUN: llvm-readobj -symbols %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
-  Machine:         EM_X86_64
+  Machine:         EM_HEXAGON
 Symbols:
   Global:
     - Name:     test
       Index:    SHN_COMMON
       Value:    0x1234
     - Name:     test2
       Index:    SHN_HEXAGON_SCOMMON
       Value:    0x1235

tools/llvm-objcopy/Object.cpp

 }
 
 void StringTableSection::finalize() { StrTabBuilder.finalize(); }
 
 void StringTableSection::writeSection(FileOutputBuffer &Out) const {
   StrTabBuilder.write(Out.getBufferStart() + Offset);
 }
 
-static bool isValidReservedSectionIndex(uint16_t Index) {
+static bool isValidReservedSectionIndex(uint16_t Index, uint16_t Machine) {
   switch (Index) {
   case SHN_ABS:
   case SHN_COMMON:
+    return true;
   case SHN_HEXAGON_SCOMMON:
   case SHN_HEXAGON_SCOMMON_2:
   case SHN_HEXAGON_SCOMMON_4:
   case SHN_HEXAGON_SCOMMON_8:
-    return true;
+    return Machine == EM_HEXAGON;
   default:
     return false;
   }
 }
 
 uint16_t Symbol::getShndx() const {
   if (DefinedIn != nullptr) {
     return DefinedIn->Index;
void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                                    SectionBase *DefinedIn, uint64_t Value,
                                    uint16_t Shndx, uint64_t Sz) {
   Symbol Sym;
   Sym.Name = Name;
   Sym.Binding = Bind;
   Sym.Type = Type;
   Sym.DefinedIn = DefinedIn;
   if (DefinedIn == nullptr) {
-    if (isValidReservedSectionIndex(Shndx))
+    if (Shndx >= SHN_LORESERVE)
       Sym.ShndxType = static_cast<SymbolShndxType>(Shndx);
     else
       Sym.ShndxType = SYMBOL_SIMPLE_INDEX;
   }
   Sym.Value = Value;
   Sym.Size = Sz;
   Sym.Index = Symbols.size();
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
void Object<ELFT>::initSymbolTable(const llvm::object::ELFFile<ELFT> &ElfFile,
 
   const Elf_Shdr &Shdr = *unwrapOrError(ElfFile.getSection(SymTab->Index));
   StringRef StrTabData = unwrapOrError(ElfFile.getStringTableForSymtab(Shdr));
 
   for (const auto &Sym : unwrapOrError(ElfFile.symbols(&Shdr))) {
     SectionBase *DefSection = nullptr;
     StringRef Name = unwrapOrError(Sym.getName(StrTabData));
     if (Sym.st_shndx >= SHN_LORESERVE) {
-      if (!isValidReservedSectionIndex(Sym.st_shndx)) {
+      if (!isValidReservedSectionIndex(Sym.st_shndx, Machine)) {
         error(
             "Symbol '" + Name +
             "' has unsupported value greater than or equal to SHN_LORESERVE: " +
             Twine(Sym.st_shndx));
       }
     } else if (Sym.st_shndx != SHN_UNDEF) {
       if (Sym.st_shndx >= Sections.size())
         error("Symbol '" + Name +