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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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
----------------
MaskRay wrote:
> 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 .
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69526