[llvm-objcopy] - Calculate the string table section sizes correctly.

[grimar]

Summary

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

Previously if string optimization occurred as a result of
StringTableBuilder's finalize() method, the size wasn't updated.

This hopefully also makes the interaction between sections during finalization
processes a bit more clear.

[jhenderson]

LGTM, with a couple of comment nits.

I think you might have flushed out another bug with string table building, based on the size changes in the two archive tests. Could you double-check, and fix or file a bug, please? I'm guessing it's just that we're adding an extra null byte.

tools/llvm-objcopy/ELF/Object.cpp

1612

when -> that

1613

I'd replace everything from the "and we need..." to the end of the comment with "which in turn affects section offsets."

[grimar]

In D59488#1433045, @jhenderson wrote:

LGTM, with a couple of comment nits.

I think you might have flushed out another bug with string table building, based on the size changes in the two archive tests. Could you double-check, and fix or file a bug, please? I'm guessing it's just that we're adding an extra null byte.

Yes, sure, I am actually looking at this :) It seems the issue is that we add a zero's symbol name here (instead of ignoring that symbol):

(https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L498)

// 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);

I am not sure yet, just made this observation during developing this patch.

[grimar]

In D59488#1433061, @grimar wrote:

In D59488#1433045, @jhenderson wrote:

LGTM, with a couple of comment nits.

I think you might have flushed out another bug with string table building, based on the size changes in the two archive tests. Could you double-check, and fix or file a bug, please? I'm guessing it's just that we're adding an extra null byte.

Yes, sure, I am actually looking at this :) It seems the issue is that we add a zero's symbol name here (instead of ignoring that symbol):

(https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L498)

// 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);

I am not sure yet, just made this observation during developing this patch.

I posted a patch for this: D59496

[jakehehrlich]

Maybe we should make "prepareForLayout" a top level operation that we can just run on all sections?

[grimar]

In D59488#1433463, @jakehehrlich wrote:

Maybe we should make "prepareForLayout" a top level operation that we can just run on all sections?

I thought about this, but the problem is call order: we should call it for symbol table earlier than for string table sections, for example.
i.e. it would not make the code to be a simple loop.

test/tools/llvm-objcopy/ELF/basic-archive-copy.test

 # CHECK-NEXT: Size: 48
 
 # CHECK:      Name: .strtab
 # CHECK-NEXT: Type: SHT_STRTAB
 # CHECK-NEXT: Flags [ (0x0)
 # CHECK-NEXT: ]
 # CHECK-NEXT: Address:
 # CHECK-NEXT: Offset:
-# CHECK-NEXT: Size: 6
+# CHECK-NEXT: Size: 5
 
 # CHECK:      Name: .shstrtab
 # CHECK-NEXT: Type: SHT_STRTAB
 # CHECK-NEXT: Flags [
 # CHECK-NEXT: ]
 # CHECK-NEXT: Address:
 # CHECK-NEXT: Offset:
 # CHECK-NEXT: Size: 38

test/tools/llvm-objcopy/ELF/cross-arch-sections-symbols.test

 # CHECK-NEXT:   Section {
 # CHECK-NEXT:     Index: 4
 # CHECK-NEXT:     Name: .strtab
 # CHECK-NEXT:     Type: SHT_STRTAB (0x3)
 # CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Address:
 # CHECK-NEXT:     Offset:
-# CHECK-NEXT:     Size: 10
+# CHECK-NEXT:     Size: 9
 # CHECK-NEXT:     Link: 0
 # CHECK-NEXT:     Info: 0
 # CHECK-NEXT:     AddressAlignment: 1
 # CHECK-NEXT:     EntrySize: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Section {
 # CHECK-NEXT:     Index: 5
 # CHECK-NEXT:     Name: .shstrtab

test/tools/llvm-objcopy/ELF/shstrtab-optimize.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
+
+## Check we have the correct size of the .shstrtab section
+## after the strings tail merge optimization.
+
+# CHECK:      Name: .shstrtab
+# CHECK-NEXT: Type: SHT_STRTAB
+# CHECK-NEXT: Flags [
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 36
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bar
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+  - Name:            .foo.bar
+    Type:            SHT_PROGBITS
+    Flags:           [ ]

test/tools/llvm-objcopy/ELF/strtab-optimize.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy %t %t2 --add-symbol='foo=1' --add-symbol='foofoo=2'
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
+
+## Check we have the correct size of the .strtab section
+## after the strings tail merge optimization.
+
+# CHECK:      Name: .strtab
+# CHECK-NEXT: Type: SHT_STRTAB
+# CHECK-NEXT: Flags [
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size: 8
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64

tools/llvm-objcopy/ELF/Object.h

 
 public:
   StringTableSection() : StrTabBuilder(StringTableBuilder::ELF) {
     Type = ELF::SHT_STRTAB;
   }
 
   void addString(StringRef Name);
   uint32_t findIndex(StringRef Name) const;
-  void finalize() override;
+  void prepareForLayout();
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 
   static bool classof(const SectionBase *S) {
     if (S->Flags & ELF::SHF_ALLOC)
       return false;
     return S->Type == ELF::SHT_STRTAB;
   }

tools/llvm-objcopy/ELF/Object.cpp

 void CompressedSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 void CompressedSection::accept(MutableSectionVisitor &Visitor) {
   Visitor.visit(*this);
 }
 
-void StringTableSection::addString(StringRef Name) {
-  StrTabBuilder.add(Name);
-  Size = StrTabBuilder.getSize();
-}
+void StringTableSection::addString(StringRef Name) { StrTabBuilder.add(Name); }
 
 uint32_t StringTableSection::findIndex(StringRef Name) const {
   return StrTabBuilder.getOffset(Name);
 }
 
-void StringTableSection::finalize() { StrTabBuilder.finalize(); }
+void StringTableSection::prepareForLayout() {
+  StrTabBuilder.finalize();
+  Size = StrTabBuilder.getSize();
+}
 
 void SectionWriter::visit(const StringTableSection &Sec) {
   Sec.StrTabBuilder.write(Out.getBufferStart() + Sec.Offset);
 }
 
 void StringTableSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
setStrTab(SecTable.getSectionOfType<StringTableSection>(
       Link,
       "Symbol table has link index of " + Twine(Link) +
           " which is not a valid index",
       "Symbol table has link index of " + Twine(Link) +
           " which is not a string table"));
 }
 
 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);
   }
   // Now we need to set the Link and Info fields.
   Link = SymbolNames->Index;
template <class ELFT> Error ELFWriter<ELFT>::finalize() {
   }
 
   // The symbol table does not update all other sections on update. For
   // instance, symbol names are not added as new symbols are added. This means
   // that some sections, like .strtab, don't yet have their final size.
   if (Obj.SymbolTable != nullptr)
     Obj.SymbolTable->prepareForLayout();
 
+  // Now when all strings are added we want to finalize string table builders,
+  // because that affects section sizes and we need to do this before assigning
+  // the offsets.
+  for (auto &Sec : Obj.sections())
+    if (auto StrTab = dyn_cast<StringTableSection>(&Sec))
+      StrTab->prepareForLayout();
+
   assignOffsets();
 
-  // Finalize SectionNames first so that we can assign name indexes.
-  if (Obj.SectionNames != nullptr)
-    Obj.SectionNames->finalize();
   // 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);