[llvm-objcopy] Address post-commit reviews of D69093

[MaskRay]

Summary

• Improve comments.
• Reorder the assignment to Obj.SectionNames before the symbol table creation code. Add a test.

[MaskRay]

Improve tests

[grimar]

LGTM with a nit, put please wait for others too.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

101

I'd suggest to use "Symbols: []" to emphasise we just want to create a ".symtab". Up to you.

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

1555

I see you just moved this part, so comment below is FTR.
A field value can never be a string table.
"does not reference a string table" would probably be better.

[jhenderson]

I think this part of one of my comments may have been missed, as I don't see a response to it anywhere:

Is there anything stopping the section header string table having the SHF_ALLOC bit set however? If not, we need a separate test for this case, I believe.

I had another thought about this, and I wonder if we might mess things up if we try to change the section names when the section header string table has SHF_ALLOC set. But this seems unlikely, so is probably a non-issue.

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

35

Do you think this check should also have an --implicit-check-not=.strtab?

96–100

This may be me being forgetful, but I don't follow how this YAML will generate anything different from the previous YAML doc.

Also, the comment for this test case says that it is checking that we prefer the string table that is not the section header table, but the test itself checks that we add it to the section header string table when the string table is not there. We need both test cases, but I'm under the impression that the latter is already covered by the previous test.

[grimar]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

96–100

This may be me being forgetful, but I don't follow how this YAML will generate anything different from the previous YAML doc.

Previous YAML does not specify sections. .strtab will be created implicitly before .shstrtab:

std::vector<StringRef> ImplicitSections;
if (Doc.Symbols)
  ImplicitSections.push_back(".symtab");
ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});

if (!Doc.DynamicSymbols.empty())
  ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});

// Insert placeholders for implicit sections that are not
// defined explicitly in YAML.
for (StringRef SecName : ImplicitSections) {
  if (DocSections.count(SecName))
    continue;

  std::unique_ptr<ELFYAML::Section> Sec = std::make_unique<ELFYAML::Section>(
      ELFYAML::Section::SectionKind::RawContent, true /*IsImplicit*/);
  Sec->Name = SecName;
  Doc.Sections.push_back(std::move(Sec));
}

This YAML specifies .shstrtab and strtab. These are not created implicitly and yaml2obj hence will follow the order requested.

[MaskRay]

Address review comments

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

96–100

Yes, the first test checks the order .strtab followed by .shstrtab while the second test checks the order .shstrtab followed by .strtab .

You may see another example in yaml2obj/section-ordering.yaml .

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

1555

The substring is not a string table occurs twice aside from here in this file. I haven't looked into it closely but one or both may be difficult to test.

Improved the error message just in this place.

[MaskRay]

In D69526#1724854, @jhenderson wrote:

I think this part of one of my comments may have been missed, as I don't see a response to it anywhere:

Is there anything stopping the section header string table having the SHF_ALLOC bit set however? If not, we need a separate test for this case, I believe.

I had another thought about this, and I wonder if we might mess things up if we try to change the section names when the section header string table has SHF_ALLOC set. But this seems unlikely, so is probably a non-issue.

I agree that changing section names may be delicate. Let's stick with:

if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
  StrTab = static_cast<StringTableSection *>(&Sec);

  // Prefer a string table that is not the section header string table, if
  // such a table exists.
  if (Obj.SectionNames != &Sec)
    break;
}

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

96–100

Okay, that makes sense. It might be a good idea to add a comment to that effect. Or just force the first case to be in the other order rather than relying on the implicit yaml2obj ordering.

[MaskRay]

test: make .shstrtab and .strtab explicit

llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test

 ## Test --add-symbol creates .symtab if it does not exist.
 
 # RUN: yaml2obj --docnum=1 %s -o %t
 
 ## If a non-SHF_ALLOC SHT_STRTAB exists but SHT_SYMTAB does not, create .symtab
 ## and set its sh_link to the SHT_STRTAB.
-# RUN: llvm-objcopy --remove-section=.symtab %t %t1
-# RUN: llvm-objcopy --add-symbol='abs1=1' %t1 %t2
-# RUN: llvm-readobj -S %t2 | FileCheck --check-prefix=SEC %s
-# RUN: llvm-readelf -s %t2 | FileCheck %s
+# RUN: llvm-objcopy --remove-section=.symtab %t %t.no.symtab
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t.no.symtab %t.add.both
+# RUN: llvm-readobj -S %t.add.both | FileCheck --check-prefix=SEC %s
+# RUN: llvm-readelf -s %t.add.both | FileCheck %s
 
 # SEC:      Index: 1
 # SEC-NEXT: Name: .strtab
 # SEC-NEXT: Type: SHT_STRTAB
 # SEC:      Index: 2
 # SEC-NEXT: Name: .shstrtab
 # SEC-NEXT: Type: SHT_STRTAB
 # SEC:      Index: 3
 # SEC-NEXT: Name: .symtab
 # SEC-NEXT: Type: SHT_SYMTAB
 # SEC-NOT:  }
 # SEC:      Link: 1
 
 # CHECK:      0: 0000000000000000 0 NOTYPE LOCAL  DEFAULT UND
 # CHECK-NEXT: 1: 0000000000000001 0 NOTYPE GLOBAL DEFAULT ABS abs1
 
 ## Don't create .symtab if --add-symbol is not specified.
-# RUN: llvm-objcopy --remove-section=.symtab --remove-section=.strtab %t %t1
-# RUN: llvm-objcopy %t1 %t2
-# RUN: llvm-readobj -S %t2 | FileCheck --implicit-check-not=.symtab /dev/null
+# RUN: llvm-objcopy --remove-section=.symtab --remove-section=.strtab %t %t.no
+# RUN: llvm-objcopy %t.no %t.nop
+# RUN: llvm-readobj -S %t.nop | FileCheck --implicit-check-not=.symtab --implicit-check-not=.strtab /dev/null
 
-## Check that we create both .strtab and .symtab.
-## We reuse the existing .shstrtab (section names) for symbol names.
+## Reuse the existing .shstrtab (section names) for symbol names.
 ## This may look strange but it works.
-# RUN: llvm-objcopy --add-symbol='abs1=1' %t1 %t2
-# RUN: llvm-readobj -S %t2 | FileCheck --check-prefix=SEC2 %s
-# RUN: llvm-readelf -s %t2 | FileCheck %s
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t.no %t.add.symtab
+# RUN: llvm-readobj -S %t.add.symtab | FileCheck --check-prefix=SEC2 %s --implicit-check-not=.strtab
+# RUN: llvm-readelf -s %t.add.symtab | FileCheck %s
 
 # SEC2:      Index: 1
 # SEC2-NEXT: Name: .shstrtab
 # SEC2-NEXT: Type: SHT_STRTAB
 # SEC2:      Index: 2
 # SEC2-NEXT: Name: .symtab
 # SEC2-NEXT: Type: SHT_SYMTAB
 # SEC2-NOT:  }
 # SEC2:      Link: 1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
-Symbols:
+Sections:
+  - Name: .strtab
+    Type: SHT_STRTAB
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+Symbols: []
+...
+
+## Check that we prefer the string table that is not the section header string table.
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: llvm-objcopy --remove-section=.symtab --remove-section=.strtab %t2 %t2.no
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t2.no %t2.add.symtab
+# RUN: llvm-readobj -S %t2.add.symtab | FileCheck --check-prefix=SEC2 %s
+# RUN: llvm-readelf -s %t2.add.symtab | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  # .shstrtab is reordered before .strtab
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+  - Name: .strtab
+    Type: SHT_STRTAB
+Symbols: []
 ...
 
 ## Check the created .symtab does not link to .dynstr (SHF_ALLOC).
-# RUN: yaml2obj --docnum=2 %s -o %t
-# RUN: llvm-objcopy --remove-section=.symtab --remove-section=.strtab %t %t1
-# RUN: llvm-objcopy --add-symbol='abs1=1' %t1 %t2
-# RUN: llvm-readobj -S %t2 | FileCheck --check-prefix=SEC3 %s
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: llvm-objcopy --remove-section=.symtab --remove-section=.strtab %t3 %t3.no
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t3.no %t3.not.dynstr
+# RUN: llvm-readobj -S %t3.not.dynstr | FileCheck --check-prefix=SEC3 %s
 
 # SEC3:      Index: 1
 # SEC3-NEXT: Name: .shstrtab
 # SEC3-NEXT: Type: SHT_STRTAB
 # SEC3:      Name: .symtab
 # SEC3-NEXT: Type: SHT_SYMTAB
 # SEC3-NOT:  }
 # SEC3:      Link: 1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 DynamicSymbols:
   - Name:    dummy
     Binding: STB_GLOBAL
-Symbols:
+Symbols: []
 ...

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

for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
     Sec.Index = Index++;
     Sec.OriginalData =
         ArrayRef<uint8_t>(ElfFile.base() + Shdr.sh_offset,
                           (Shdr.sh_type == SHT_NOBITS) ? 0 : Shdr.sh_size);
   }
 }
 
 template <class ELFT> void ELFBuilder<ELFT>::readSections(bool EnsureSymtab) {
+  uint32_t ShstrIndex = ElfFile.getHeader()->e_shstrndx;
+  if (ShstrIndex == SHN_XINDEX)
+    ShstrIndex = unwrapOrError(ElfFile.getSection(0))->sh_link;
+
+  if (ShstrIndex == SHN_UNDEF)
+    Obj.HadShdrs = false;
+  else
+    Obj.SectionNames =
+        Obj.sections().template getSectionOfType<StringTableSection>(
+            ShstrIndex,
+            "e_shstrndx field value " + Twine(ShstrIndex) + " in elf header " +
+                " is invalid",
+            "e_shstrndx field value " + Twine(ShstrIndex) + " in elf header " +
+                " does not reference a string table");
+
   // If a section index table exists we'll need to initialize it before we
   // initialize the symbol table because the symbol table might need to
   // reference it.
   if (Obj.SectionIndexTable)
     Obj.SectionIndexTable->initialize(Obj.sections());
 
   // Now that all of the sections have been added we can fill out some extra
   // details about symbol tables. We need the symbol table filled out before
   // any relocations.
   if (Obj.SymbolTable) {
     Obj.SymbolTable->initialize(Obj.sections());
     initSymbolTable(Obj.SymbolTable);
   } else if (EnsureSymtab) {
-    // Reuse the existing SHT_STRTAB section if exists.
+    // Reuse an existing SHT_STRTAB section if it exists.
     StringTableSection *StrTab = nullptr;
     for (auto &Sec : Obj.sections()) {
       if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
         StrTab = static_cast<StringTableSection *>(&Sec);
 
-        // Prefer .strtab to .shstrtab.
+        // Prefer a string table that is not the section header string table, if
+        // such a table exists.
         if (Obj.SectionNames != &Sec)
           break;
       }
     }
     if (!StrTab)
       StrTab = &Obj.addSection<StringTableSection>();
 
     SymbolTableSection &SymTab = Obj.addSection<SymbolTableSection>();
if (auto RelSec = dyn_cast<RelocationSection>(&Sec)) {
                         unwrapOrError(ElfFile.rels(Shdr)));
       else
         initRelocations(RelSec, Obj.SymbolTable,
                         unwrapOrError(ElfFile.relas(Shdr)));
     } else if (auto GroupSec = dyn_cast<GroupSection>(&Sec)) {
       initGroupSection(GroupSec);
     }
   }
-
-  uint32_t ShstrIndex = ElfFile.getHeader()->e_shstrndx;
-  if (ShstrIndex == SHN_XINDEX)
-    ShstrIndex = unwrapOrError(ElfFile.getSection(0))->sh_link;
-
-  if (ShstrIndex == SHN_UNDEF)
-    Obj.HadShdrs = false;
-  else
-    Obj.SectionNames =
-        Obj.sections().template getSectionOfType<StringTableSection>(
-            ShstrIndex,
-            "e_shstrndx field value " + Twine(ShstrIndex) + " in elf header " +
-                " is invalid",
-            "e_shstrndx field value " + Twine(ShstrIndex) + " in elf header " +
-                " is not a string table");
 }
 
 template <class ELFT> void ELFBuilder<ELFT>::build(bool EnsureSymtab) {
   readSectionHeaders();
   findEhdrOffset();
 
   // The ELFFile whose ELF headers and program headers are copied into the
   // output file. Normally the same as ElfFile, but if we're extracting a