[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

222 ↗

(On Diff #113580)

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 ↗

(On Diff #113615)

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

239 ↗

(On Diff #113615)

It's >=, not >.

tools/llvm-objcopy/Object.h

119 ↗

(On Diff #113615)

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 ↗

(On Diff #113615)

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

2 ↗

(On Diff #113920)

I think we should also check the error message here.

tools/llvm-objcopy/Object.cpp

93 ↗

(On Diff #113920)

uint32_t -> uint16_t (to match st_shndx size).

123 ↗

(On Diff #113920)

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

tools/llvm-objcopy/Object.h

152 ↗

(On Diff #113920)

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

15 ↗

(On Diff #114053)

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

tools/llvm-objcopy/Object.cpp

110 ↗

(On Diff #114053)

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!

llvm/trunk/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:]

llvm/trunk/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
+Symbols:
+  Global:
+    - Name:     test
+      Index:    SHN_COMMON
+      Value:    0x1234
+    - Name:     test2
+      Index:    SHN_HEXAGON_SCOMMON
+      Value:    0x1235
+    - Name:     test3
+      Index:    SHN_HEXAGON_SCOMMON_2
+      Value:    0x1236
+    - Name:     test4
+      Index:    SHN_HEXAGON_SCOMMON_4
+      Value:    0x1237
+    - Name:     test5
+      Index:    SHN_HEXAGON_SCOMMON_8
+      Value:    0x1238
+
+#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: Common (0xFFF2)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test2
+#CHECK-NEXT:    Value: 0x1235
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Processor Specific (0xFF00)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test3
+#CHECK-NEXT:    Value: 0x1236
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Processor Specific (0xFF02)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test4
+#CHECK-NEXT:    Value: 0x1237
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Processor Specific (0xFF03)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test5
+#CHECK-NEXT:    Value: 0x1238
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Processor Specific (0xFF04)
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

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

+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy %t %t2 2>&1 >/dev/null | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Symbols:
+  Global:
+    - Name:     test
+      Index:    0xff05
+
+# CHECK: [[_:.*]] Symbol 'test' has unsupported value greater than or equal to SHN_LORESERVE: 65285

llvm/trunk/tools/llvm-objcopy/Object.h

public:
   uint32_t findIndex(llvm::StringRef Name) const;
   void finalize() override;
   void writeSection(llvm::FileOutputBuffer &Out) const override;
   static bool classof(const SectionBase *S) {
     return S->Type == llvm::ELF::SHT_STRTAB;
   }
 };
 
+// Symbols have a st_shndx field that normally stores an index but occasionally
+// stores a different special value. This enum keeps track of what the st_shndx
+// field means. Most of the values are just copies of the special SHN_* values.
+// SYMBOL_SIMPLE_INDEX means that the st_shndx is just an index of a section.
+enum SymbolShndxType {
+  SYMBOL_SIMPLE_INDEX = 0,
+  SYMBOL_ABS = llvm::ELF::SHN_ABS,
+  SYMBOL_COMMON = llvm::ELF::SHN_COMMON,
+  SYMBOL_HEXAGON_SCOMMON = llvm::ELF::SHN_HEXAGON_SCOMMON,
+  SYMBOL_HEXAGON_SCOMMON_2 = llvm::ELF::SHN_HEXAGON_SCOMMON_2,
+  SYMBOL_HEXAGON_SCOMMON_4 = llvm::ELF::SHN_HEXAGON_SCOMMON_4,
+  SYMBOL_HEXAGON_SCOMMON_8 = llvm::ELF::SHN_HEXAGON_SCOMMON_8,
+};
+
 struct Symbol {
   uint8_t Binding;
   SectionBase *DefinedIn;
+  SymbolShndxType ShndxType;
   uint32_t Index;
   llvm::StringRef Name;
   uint32_t NameIndex;
   uint64_t Size;
   uint8_t Type;
   uint64_t Value;
+
+  uint16_t getShndx() const;
 };
 
 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, uint16_t Shndx,
+                 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;
   }
 };
 

llvm/trunk/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) {
+  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;
+  }
+}
+
+uint16_t Symbol::getShndx() const {
+  if (DefinedIn != nullptr) {
+    return DefinedIn->Index;
+  }
+  switch (ShndxType) {
+  // This means that we don't have a defined section but we do need to
+  // output a legitimate section index.
+  case SYMBOL_SIMPLE_INDEX:
+    return SHN_UNDEF;
+  case SYMBOL_ABS:
+  case SYMBOL_COMMON:
+  case SYMBOL_HEXAGON_SCOMMON:
+  case SYMBOL_HEXAGON_SCOMMON_2:
+  case SYMBOL_HEXAGON_SCOMMON_4:
+  case SYMBOL_HEXAGON_SCOMMON_8:
+    return static_cast<uint16_t>(ShndxType);
+  }
+  llvm_unreachable("Symbol with invalid ShndxType encountered");
+}
+
 void SymbolTableSection::addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                                    SectionBase *DefinedIn, uint64_t Value,
-                                   uint64_t Sz) {
+                                   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))
+      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));
   Size += this->EntrySize;
 }
 
 void SymbolTableSection::finalize() {
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->getShndx();
     ++Sym;
   }
 }
 
 template <class ELFT> void RelocationSection<ELFT>::finalize() {
   this->Link = Symbols->Index;
   this->Info = SecToApplyRel->Index;
 }
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 (!isValidReservedSectionIndex(Sym.st_shndx)) {
+        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 +
               "' 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>
 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, false> &Rel) {}
 
 template <class ELFT>
 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) {