[llvm-objcopy] Fill .symtab_shndx section correctly

[evgeny777]

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=41461

However I have problem with test case, which requires 19MB binary file. Any help is appreciated.

[jhenderson]

Test case?

tools/llvm-objcopy/ELF/Object.cpp

1669 ↗

(On Diff #194646)

that why -> so

1670 ↗

(On Diff #194646)

fill index table -> fill the index table

tools/llvm-objcopy/ELF/Object.h

484 ↗

(On Diff #194646)

Is this assert correct? What happens if you have a very large object with many sections, but no symbols. Will that result in an empty SectionIndexSection?

Assuming it's correct, I would find it more readable for this to be assert(Size > 0);

[evgeny777]

Test case?

Have you seen bug description?
The problem with test case is that it requires file with large number of sections of which at least some should have relocation sections.
I have 19Mb object file generated by clang, but I'm not sure if using it as test case is a good idea ...

[evgeny777]

tools/llvm-objcopy/ELF/Object.h

484 ↗

(On Diff #194646)

Is this assert correct? What happens if you have a very large object with many sections, but no symbols

This function will never be called then, because Index is actually index of a section where symbol is defined.

Assuming it's correct, I would find it more readable for this to be assert(Size > 0);

Makes sense

[jhenderson]

In D60555#1462406, @evgeny777 wrote:

Test case?

Have you seen bug description?
The problem with test case is that it requires file with large number of sections of which at least some should have relocation sections.
I have 19Mb object file generated by clang, but I'm not sure if using it as test case is a good idea ...

Sorry, I skipped over that somehow (I think I jumped straight from the bug). We already have at least one test that uses many sections (see many-sections.test). That unzips a pre-built object to achieve this. One option might be to update that object file (and possibly the test), if it can satisfy the prerequisites you need.

[evgeny777]

That was helpful, thanks! I'll update diff shortly

[arichardson]

In D60555#1462414, @jhenderson wrote:

In D60555#1462406, @evgeny777 wrote:

Test case?

Have you seen bug description?
The problem with test case is that it requires file with large number of sections of which at least some should have relocation sections.
I have 19Mb object file generated by clang, but I'm not sure if using it as test case is a good idea ...

Sorry, I skipped over that somehow (I think I jumped straight from the bug). We already have at least one test that uses many sections (see many-sections.test). That unzips a pre-built object to achieve this. One option might be to update that object file (and possibly the test), if it can satisfy the prerequisites you need.

Could we use llvm-mc and some loop macros to generate a binary with the required amount of sections?
I did that for a testcase that needed 32768 symbols: https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/mips-out-of-bounds-call16-reloc.s

[evgeny777]

Added test case, addressed comments.

[jhenderson]

LGTM.

In D60555#1462432, @arichardson wrote:

Could we use llvm-mc and some loop macros to generate a binary with the required amount of sections?
I did that for a testcase that needed 32768 symbols: https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/mips-out-of-bounds-call16-reloc.s

Good tip, thanks. It's probably worth bearing in mind in case we have any more tests along these lines that the existing pre-canned binary can't be used for.

test/tools/llvm-objcopy/ELF/many-sections.test

56 ↗

(On Diff #194655)

Looks to me like you've hit a bug here. This should actually be index 0xff01 (going via SHN_XINDEX), not PRC[0xff01] which is a reserved value. readelf will print 65281 here. Could you file a bug against llvm-readobj, please?

You might want to consider using llvm-readobj instead of llvm-readelf, as this doesn't have that bug.

[evgeny777]

Looks to me like you've hit a bug here

Are you sure it's a bug? According to sources it looks like some extension to original readelf output:

// Find if:
// Processor specific
if (SectionIndex >= ELF::SHN_LOPROC && SectionIndex <= ELF::SHN_HIPROC)
  return std::string("PRC[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// OS specific
if (SectionIndex >= ELF::SHN_LOOS && SectionIndex <= ELF::SHN_HIOS)
  return std::string("OS[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// Architecture reserved:
if (SectionIndex >= ELF::SHN_LORESERVE &&
    SectionIndex <= ELF::SHN_HIRESERVE)
  return std::string("RSV[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";

Anyway I'll switch to llvm-readobj in the test

[jhenderson]

In D60555#1464071, @evgeny777 wrote:

Looks to me like you've hit a bug here

Are you sure it's a bug? According to sources it looks like some extension to original readelf output:

// Find if:
// Processor specific
if (SectionIndex >= ELF::SHN_LOPROC && SectionIndex <= ELF::SHN_HIPROC)
  return std::string("PRC[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// OS specific
if (SectionIndex >= ELF::SHN_LOOS && SectionIndex <= ELF::SHN_HIOS)
  return std::string("OS[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";
// Architecture reserved:
if (SectionIndex >= ELF::SHN_LORESERVE &&
    SectionIndex <= ELF::SHN_HIRESERVE)
  return std::string("RSV[0x") +
         to_string(format_hex_no_prefix(SectionIndex, 4)) + "]";

Anyway I'll switch to llvm-readobj in the test

Yes, it's a bug. Where the original st_shndx field is between SHN_LORESERVE and SHN_HIRESERVE (and not SHN_XINDEX), it should be decorated in this way. In all other cases it shouldn't be. It's a little complicated to explain in detail, but I'll do my best. If we look at the section header table, there are sections with index 0xff00, 0xff01, etc. Thus when we generally refer to sections with index 0xff00 etc we are talking about those sections. The symbol st_shndx field (i.e. the field indicating the symbol's section) however is only 16 bits, and the upper few values are reserved for various purposes. The SHN_XINDEX value (0xffff) in this context means look up the section index in the SHT_SYMTAB_SHNDX section instead. Other values in this range have other meanings (e.g. absolute and common symbols). If the real section index for the symbol's section is >= SHN_LORESERVE, it is impossible to represent it in directly in st_shndx, so it will be SHN_XINDEX.

llvm-readelf should print the real section index in the section index column, or a special interpretation where the value in the st_shndx field is a reserved value, such as SHN_ABS. The code in llvm-readelf should only do that special interpretation for the raw st_shndx field value, not the interpreted value (i.e. the value after lookups in SHT_SYMTAB_SHNDX) which I think is what it is doing (though I haven't looked in depth at the code yet to be 100% sure).

[bd1976llvm]

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

492

Let's stick to SectionIndexTable =! nullptr given line 501

llvm/trunk/test/tools/llvm-objcopy/ELF/many-sections.test

 RUN: %python %p/../Inputs/ungzip.py %p/Inputs/many-sections.o.gz > %t
 RUN: llvm-objcopy %t %t2
 RUN: llvm-readobj --file-headers %t2 | FileCheck --check-prefix=EHDR %s
 RUN: llvm-readobj --sections %t2 | FileCheck --check-prefix=SECS %s
 RUN: llvm-readobj --symbols %t2 | grep "Symbol {" | wc -l | FileCheck --check-prefix=SYMS %s
+RUN: llvm-readobj -symbols %t2 | FileCheck %s --check-prefix=SYM_SEC_IDS
 
 EHDR:      Format: ELF64-x86-64
 EHDR-NEXT: Arch: x86_64
 EHDR-NEXT: AddressSize: 64bit
 EHDR-NEXT: LoadName:
 EHDR-NEXT: ElfHeader {
 EHDR-NEXT:   Ident {
 EHDR-NEXT:     Magic: (7F 45 4C 46)

 # There should be #syms * EntrySize bytes.
 SECS-NEXT: Size: 261136
 SECS-NEXT: Link: 65285
 SECS-NEXT: Info:
 SECS-NEXT: AddressAlignment: 4
 SECS-NEXT: EntrySize: 4
 SECS: Index: 65287
 SYMS: 65284
+
+SYM_SEC_IDS:         Section: s9 (0xFEFF)
+SYM_SEC_IDS-NEXT:  }
+SYM_SEC_IDS-NEXT:  Symbol {
+SYM_SEC_IDS-NEXT:    Name: s9 (37)
+SYM_SEC_IDS-NEXT:    Value: 0x0
+SYM_SEC_IDS-NEXT:    Size: 0
+SYM_SEC_IDS-NEXT:    Binding: Local (0x0)
+SYM_SEC_IDS-NEXT:    Type: None (0x0)
+SYM_SEC_IDS-NEXT:    Other: 0
+SYM_SEC_IDS-NEXT:    Section: s9 (0xFF01)
+SYM_SEC_IDS-NEXT:  }
+SYM_SEC_IDS-NEXT:  Symbol {
+SYM_SEC_IDS-NEXT:    Name: s9 (37)
+SYM_SEC_IDS-NEXT:    Value: 0x0
+SYM_SEC_IDS-NEXT:    Size: 0
+SYM_SEC_IDS-NEXT:    Binding: Local (0x0)
+SYM_SEC_IDS-NEXT:    Type: None (0x0)
+SYM_SEC_IDS-NEXT:    Other: 0
+SYM_SEC_IDS-NEXT:    Section: s9 (0xFF00)
+SYM_SEC_IDS-NEXT:  }
+SYM_SEC_IDS-NEXT:  Symbol {
+SYM_SEC_IDS-NEXT:    Name: s9 (37)
+SYM_SEC_IDS-NEXT:    Value: 0x0
+SYM_SEC_IDS-NEXT:    Size: 0
+SYM_SEC_IDS-NEXT:    Binding: Local (0x0)
+SYM_SEC_IDS-NEXT:    Type: None (0x0)
+SYM_SEC_IDS-NEXT:    Other: 0
+SYM_SEC_IDS-NEXT:    Section: s9 (0x1982)
+SYM_SEC_IDS-NEXT:  }
+SYM_SEC_IDS-NEXT:  Symbol {
+SYM_SEC_IDS-NEXT:    Name: s9 (37)
+SYM_SEC_IDS-NEXT:    Value: 0x0
+SYM_SEC_IDS-NEXT:    Size: 0
+SYM_SEC_IDS-NEXT:    Binding: Local (0x0)
+SYM_SEC_IDS-NEXT:    Type: None (0x0)
+SYM_SEC_IDS-NEXT:    Other: 0
+SYM_SEC_IDS-NEXT:    Section: s9 (0xFF03)
+SYM_SEC_IDS-NEXT:  }
+SYM_SEC_IDS-NEXT:  Symbol {
+SYM_SEC_IDS-NEXT:    Name: s9 (37)
+SYM_SEC_IDS-NEXT:    Value: 0x0
+SYM_SEC_IDS-NEXT:    Size: 0
+SYM_SEC_IDS-NEXT:    Binding: Local (0x0)
+SYM_SEC_IDS-NEXT:    Type: None (0x0)
+SYM_SEC_IDS-NEXT:    Other: 0
+SYM_SEC_IDS-NEXT:    Section: s9 (0xFF04)
+SYM_SEC_IDS-NEXT:  }
+

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

 
 private:
   std::vector<uint32_t> Indexes;
   SymbolTableSection *Symbols = nullptr;
 
 public:
   virtual ~SectionIndexSection() {}
   void addIndex(uint32_t Index) {
-    Indexes.push_back(Index);
-    Size += 4;
-  }
+    assert(Size > 0);
+    Indexes.push_back(Index);    
+  }
+
+  void reserve(size_t NumSymbols) {
+    Indexes.reserve(NumSymbols);
+    Size = NumSymbols * 4;
+  }  
   void setSymTab(SymbolTableSection *SymTab) { Symbols = SymTab; }
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 
   SectionIndexSection() {
     Name = ".symtab_shndx";
void addSymbol(Twine Name, uint8_t Bind, uint8_t Type, SectionBase *DefinedIn,
                  uint64_t Size);
   void prepareForLayout();
   // An 'empty' symbol table still contains a null symbol.
   bool empty() const { return Symbols.size() == 1; }
   void setShndxTable(SectionIndexSection *ShndxTable) {
     SectionIndexTable = ShndxTable;
   }
   const SectionIndexSection *getShndxTable() const { return SectionIndexTable; }
+  void fillShndxTable();
   const SectionBase *getStrTab() const { return SymbolNames; }
   const Symbol *getSymbolByIndex(uint32_t Index) const;
   Symbol *getSymbolByIndex(uint32_t Index);
   void updateSymbols(function_ref<void(Symbol &)> Callable);
 
   Error removeSectionReferences(
       function_ref<bool(const SectionBase *)> ToRemove) override;
   void initialize(SectionTableRef SecTable) override;

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

if (Sym->Binding == STB_LOCAL)
       MaxLocalIndex = std::max(MaxLocalIndex, Sym->Index);
   }
   // Now we need to set the Link and Info fields.
   Link = SymbolNames->Index;
   Info = MaxLocalIndex + 1;
 }
 
 void SymbolTableSection::prepareForLayout() {
-  // Add all potential section indexes before file layout so that the section
-  // index section has the approprite size.
-  if (SectionIndexTable != nullptr) {
-    for (const auto &Sym : Symbols) {
-      if (Sym->DefinedIn != nullptr && Sym->DefinedIn->Index >= SHN_LORESERVE)
-        SectionIndexTable->addIndex(Sym->DefinedIn->Index);
-      else
-        SectionIndexTable->addIndex(SHN_UNDEF);
-    }
-  }
-  // 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);
-}
+  // Reserve proper amount of space in section index table, so we can
+  // layout sections correctly. We will fill the table with correct
+  // indexes later in fillShdnxTable.
+  if (SectionIndexTable)  
+    SectionIndexTable->reserve(Symbols.size());
+  // 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 SymbolTableSection::fillShndxTable() {
+  if (SectionIndexTable == nullptr)
+    return;
+  // Fill section index table with real section indexes. This function must
+  // be called after assignOffsets.
 
 const Symbol *SymbolTableSection::getSymbolByIndex(uint32_t Index) const {
   if (Symbols.size() <= Index)
     error("Invalid symbol index: " + Twine(Index));
   return Symbols[Index].get();
 }
 
 Symbol *SymbolTableSection::getSymbolByIndex(uint32_t Index) {
template <class ELFT> Error ELFWriter<ELFT>::finalize() {
   // Now that all strings are added we want to finalize string table builders,
   // because that affects section sizes which in turn affects section offsets.
   for (auto &Sec : Obj.sections())
     if (auto StrTab = dyn_cast<StringTableSection>(&Sec))
       StrTab->prepareForLayout();
 
   assignOffsets();
 
+  // layoutSections could have modified section indexes, so we need
+  // to fill the index table after assignOffsets.
+  if (Obj.SymbolTable != nullptr)
+    Obj.SymbolTable->fillShndxTable();
+
   // Finally now that all offsets and indexes have been set we can finalize any
   // remaining issues.
   uint64_t Offset = Obj.SHOffset + sizeof(Elf_Shdr);
   for (auto &Section : Obj.sections()) {
     Section.HeaderOffset = Offset;
     Offset += sizeof(Elf_Shdr);
     if (WriteSectionHeaders)
       Section.NameIndex = Obj.SectionNames->findIndex(Section.Name);