[PATCH] D69526: [llvm-objcopy] Address post-commit reviews of D69093

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:69-73
+Sections:
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+  - Name: .strtab
+    Type: SHT_STRTAB
----------------
grimar wrote:
> jhenderson wrote:
> > 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.
> > 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.
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 .


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1544
+            "e_shstrndx field value " + Twine(ShstrIndex) + " in elf header " +
+                " is not a string table");
+
----------------
grimar wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69526/new/

https://reviews.llvm.org/D69526