[llvm-objcopy] Add support for special section indexes in symbol table greater than SHN_LORESERVE

[jakehehrlich]

Summary

As is indexes above SHN_LORESERVE will not be handled correctly because they'll be treated as indexes of sections rather than special values that should just be copied. This change adds support to copy them though.

[mcgrathr]

tools/llvm-objcopy/Object.cpp

236–242

This fails to handle SHN_XINDEX correctly. See its handling elsewhere in the code base, or read the ELF spec.

It's probably OK to just pass all other >= SHN_LORESERVE values through blindly, but I'm not sure it's the best idea.
Their meaning and appropriate handling depends on each specific value defined by a particular machine or OS.
For all the ones I'm familiar with, just passing it through is all you need to do. But I'm not entirely sanguine about assuming that uniformly.
I think it's probably better to handle each known case individually, and error out for values we don't expect.

[jakehehrlich]

1. Followed Roland's recomendation and throw an error on SHN_XINDEX and I only support specific values. If an unsupported value is used llvm-objcopy should now throw an error.

[mcgrathr]

I'm not closely familiar with this code base, so it should probably still get some review from someone who has reviewed the objcopy implementation so far.
But just looking at the apparent logic in this change, it looks OK to me with the caveats noted below.

tools/llvm-objcopy/Object.cpp

200

This seems an oddly general-sounding name for this.
Perhaps isValidReservedSectionIndex?

239

It's >=, not >.

tools/llvm-objcopy/Object.h

119

Since this is currently not actually the section index per se, I'd prefer not to call it that.
It's the st_shndx value, but it's not an index if it's a reserved value.
In fact, arguably it should be ElfNN_Section type, i.e. uint16_t.
And have a comment about the meaning.
When you actually support 32-bit section indices, you can have a field called SectionIndex that's 32 bits.
(You'll still need to separately represent when st_shndx is a reserved value, so as to distinguish it from when st_shndx is SHN_XINDEX and the 32-bit SHT_SYMTAB_SHNDX entry happens to be in the [SHN_LORESERVE,SHN_HIRESERVE] range.)

[jhenderson]

In addition to the already-added SHN_ABS test, could we please have a test for at least SHN_COMMON, and possibly the hexagon values as well.

tools/llvm-objcopy/Object.h

119

As the Symbol has a reference to the corresponding section via "DefinedIn", I don't think we need to have separate fields for a section index and the st_shndx value. Indeed, with exception of the special indexes, we don't need the section index here at all. Therefore, I think it might make more sense to have a little enum that can be converted from and to, which only supports the special values we support (plus a "normal symbol" value). It would then be the responsibility of finalize and the caller of addSymbol to map between the two values.

[jakehehrlich]

1. Fixed function name to be isValidReservedSectionIndex
2. Changed message to correctly indicate that it should be >= and not >
3. Switched to using an enum style solution. I added a method, getShndx that figures out what the correct value of st_shndx should be. I also switched how addSymbol works to account for this.
4. Added requested tests

[jhenderson]

test/tools/llvm-objcopy/section-index-unsupported.test

3

I think we should also check the error message here.

tools/llvm-objcopy/Object.cpp

93

uint32_t -> uint16_t (to match st_shndx size).

123

I think this should have an llvm_unreachable clause at the end.

tools/llvm-objcopy/Object.h

136–137

SectionIndex -> Shndx. It should also be a uint16_t, since that is the st_shndx field size.

[jakehehrlich]

Made changes requested by James

[jhenderson]

LGTM, with the two minor changes I've suggested inline.

test/tools/llvm-objcopy/section-index-unsupported.test

16

I don't think you need the "[[_:.*]]". The check doesn't need to match the whole line.

tools/llvm-objcopy/Object.cpp

110

As the if returns, I think the LLVM style is to not have the "else" here - it just becomes the rest of the function.

[jakehehrlich]

Fixed those two things!

test/tools/llvm-objcopy/abs-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
+Symbols:
+  Global:
+    - Name:     test
+      Index:    SHN_ABS
+      Value:    0x1234
+
+#CHECK:     Symbols [
+#CHECK-NEXT:   Symbol {
+#CHECK-NEXT:    Name:
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local (0x0)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined (0x0)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test
+#CHECK-NEXT:    Value: 0x1234
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Absolute (0xFFF1)
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

test/tools/llvm-objcopy/section-index-unsupported.test

+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy %t %t2
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Symbols:
+  Global:
+    - Name:     test
+      Index:    0xff05

tools/llvm-objcopy/Object.h

public:
   static bool classof(const SectionBase *S) {
     return S->Type == llvm::ELF::SHT_STRTAB;
   }
 };
 
 struct Symbol {
   uint8_t Binding;
   SectionBase *DefinedIn;
+  uint32_t SectionIndex;
   uint32_t Index;
   llvm::StringRef Name;
   uint32_t NameIndex;
   uint64_t Size;
   uint8_t Type;
   uint64_t Value;
 };
 
 class SymbolTableSection : public SectionBase {
 protected:
   std::vector<std::unique_ptr<Symbol>> Symbols;
   StringTableSection *SymbolNames;
 
 public:
   void setStrTab(StringTableSection *StrTab) { SymbolNames = StrTab; }
   void addSymbol(llvm::StringRef Name, uint8_t Bind, uint8_t Type,
-                 SectionBase *DefinedIn, uint64_t Value, uint64_t Sz);
+                 SectionBase *DefinedIn, uint64_t Value, uint32_t SectionIndex,
+                 uint64_t Sz);
   void addSymbolNames();
   const Symbol *getSymbolByIndex(uint32_t Index) const;
   void finalize() override;
   static bool classof(const SectionBase *S) {
     return S->Type == llvm::ELF::SHT_SYMTAB;
   }
 };
 

tools/llvm-objcopy/Object.cpp

 }
 
 void StringTableSection::finalize() { StrTabBuilder.finalize(); }
 
 void StringTableSection::writeSection(FileOutputBuffer &Out) const {
   StrTabBuilder.write(Out.getBufferStart() + Offset);
 }
 
 void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                                    SectionBase *DefinedIn, uint64_t Value,
-                                   uint64_t Sz) {
+                                   uint32_t SectionIndex, uint64_t Sz) {
   Symbol Sym;
   Sym.Name = Name;
   Sym.Binding = Bind;
   Sym.Type = Type;
   Sym.DefinedIn = DefinedIn;
   Sym.Value = Value;
   Sym.Size = Sz;
   Sym.Index = Symbols.size();
+  Sym.SectionIndex = SectionIndex;
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
   Size += this->EntrySize;
 }
 
 void SymbolTableSection::finalize() {
   // Make sure SymbolNames is finalized before getting name indexes.
   SymbolNames->finalize();
 
   uint32_t MaxLocalIndex = 0;
   for (auto &Sym : Symbols) {
     Sym->NameIndex = SymbolNames->findIndex(Sym->Name);
     if (Sym->Binding == STB_LOCAL)
       MaxLocalIndex = std::max(MaxLocalIndex, Sym->Index);
+
+    if (Sym->DefinedIn)
+      Sym->SectionIndex = Sym->DefinedIn->Index;
   }
   // Now we need to set the Link and Info fields.
   Link = SymbolNames->Index;
   Info = MaxLocalIndex + 1;
 }
 
 void SymbolTableSection::addSymbolNames() {
   // Add all of our strings to SymbolNames so that SymbolNames has the right
   // size before layout is decided.
   for (auto &Sym : Symbols)
     SymbolNames->addString(Sym->Name);
void SymbolTableSectionImpl<ELFT>::writeSection(
   typename ELFT::Sym *Sym = reinterpret_cast<typename ELFT::Sym *>(Buf);
   // Loop though symbols setting each entry of the symbol table.
   for (auto &Symbol : Symbols) {
     Sym->st_name = Symbol->NameIndex;
     Sym->st_value = Symbol->Value;
     Sym->st_size = Symbol->Size;
     Sym->setBinding(Symbol->Binding);
     Sym->setType(Symbol->Type);
-    if (Symbol->DefinedIn)
-      Sym->st_shndx = Symbol->DefinedIn->Index;
-    else
-      Sym->st_shndx = SHN_UNDEF;
+    Sym->st_shndx = Symbol->SectionIndex;
     ++Sym;
   }
 }
 
 // Returns true IFF a section is wholly inside the range of a segment
 static bool sectionWithinSegment(const SectionBase &Section,
                                  const Segment &Segment) {
   // If a section is empty it should be treated like it has a size of 1. This is
for (auto &Section : Sections) {
             Section->ParentSegment->Offset > Seg.Offset) {
           Section->ParentSegment = &Seg;
         }
       }
     }
   }
 }
 
+bool isSupportedSectionIndex(uint32_t Index) {
+  switch(Index) {
+  case SHN_ABS:
+  case SHN_COMMON:
+  case SHN_HEXAGON_SCOMMON:
+  case SHN_HEXAGON_SCOMMON_2:
+  case SHN_HEXAGON_SCOMMON_4:
+  case SHN_HEXAGON_SCOMMON_8:
+    return true;
+  default:
+    return false;
+  }
+}
+
 template <class ELFT>
 void Object<ELFT>::initSymbolTable(const llvm::object::ELFFile<ELFT> &ElfFile,
                                    SymbolTableSection *SymTab) {
 
   SymTab->Size = 0;
   if (SymbolTable->Link - 1 >= Sections.size())
     error("Symbol table has link index of " + Twine(SymbolTable->Link) +
           " which is not a valid index");
 
   if (auto StrTab =
           dyn_cast<StringTableSection>(Sections[SymbolTable->Link - 1].get()))
     SymTab->setStrTab(StrTab);
   else
     error("Symbol table has link index of " + Twine(SymbolTable->Link) +
           "which is not a string table");
 
   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_UNDEF) {
+    if (Sym.st_shndx >= SHN_LORESERVE) {
+      if (!isSupportedSectionIndex(Sym.st_shndx)) {
+        error("Symbol '" + Name +
+              "' has unsupported value greater than SHN_LORESERVE: " +
+              Twine(Sym.st_shndx));
+      }
+    } else if (Sym.st_shndx != SHN_UNDEF) {
       if (Sym.st_shndx >= Sections.size())
         error("Symbol '" + Name +
               "' is defined in invalid section with index " +
               Twine(Sym.st_shndx));
       DefSection = Sections[Sym.st_shndx - 1].get();
     }
     SymTab->addSymbol(Name, Sym.getBinding(), Sym.getType(), DefSection,
-                      Sym.getValue(), Sym.st_size);
+                      Sym.getValue(), Sym.st_shndx, Sym.st_size);
   }
 }
 
 template <class ELFT>
 std::unique_ptr<SectionBase>
 Object<ELFT>::makeSection(const llvm::object::ELFFile<ELFT> &ElfFile,
                           const Elf_Shdr &Shdr) {
   ArrayRef<uint8_t> Data;