[yaml2obj] - Change how we handle implicit sections.

[grimar]

Summary

Instead of having the special list of implicit sections,
that are mixed with the sections read from YAML on late
stages, I just create the placeholders and add them to
the main sections list early.

That allows to significantly simplify the code.

[grimar]

As an alternative if we do not want to make document non-const, I can introduce a new list of Section *
and populate it using Doc.Sections and new sections. yaml2elf will refer to this new list instead of Doc.Sections everywhere.
I did not try, but this also should work.

(i.e. it will be almost the same patch like now, just needs an additional list(s). I am not sure how much important to keep the const here, this approach is a bit simpler)

[MaskRay]

include/llvm/ObjectYAML/ELFYAML.h

153 ↗

(On Diff #210843)

Prefer in-class default member initializer bool IsImplicit = false;

But I think changing the signature to Section(SectionKind Kind, bool IsImplicit) may look better (you'll have to fix 9 ctor calls in this file).

tools/yaml2obj/yaml2elf.cpp

205 ↗

(On Diff #210843)

Doc.Sections.emplace_back(ELFYAML::Section::SectionKind::RawContent, true)
Doc.Sections.back()->Name = SecName; // After llvm migrates to C++17, we can leverage the reference return type of emplace_back.
```;

[grimar]

• Addressed review comments.

tools/yaml2obj/yaml2elf.cpp

205 ↗

(On Diff #210843)

Doc.Sections.emplace_back(ELFYAML::Section::SectionKind::RawContent, true)

I can't do this. Doc.Sections is a vector of unique_ptr<Section> And compiler reasonably barks:

1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xmemory0(881): error C2664: 'std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)': cannot convert argument 1 from '_Ty' to 'llvm::ELFYAML::Section *'
1> with
1> [
1> _Ty=llvm::ELFYAML::Section
1> ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xmemory0(881): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\vector(902): note: see reference to function template instantiation 'void std::_Default_allocator_traits<_Alloc>::construct<_Ty,llvm::ELFYAML::Section::SectionKind,bool>(_Alloc &,_Objty *const ,llvm::ELFYAML::Section::SectionKind &&,bool &&)' being compiled
1> with
1> [
1> _Alloc=std::allocator<std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>>,
1> _Ty=std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>,
1> _Objty=std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>
1> ]

[jhenderson]

include/llvm/ObjectYAML/ELFYAML.h

155 ↗

(On Diff #211025)

Perhaps worth considering making IsImplicit a default argument?

tools/yaml2obj/yaml2elf.cpp

195 ↗

(On Diff #211025)

Just a thought, not really related to this change, but shouldn't the presence of .symtab and .strtab be dependent on there being any symbols defined (just like .dynsym/.dynstr)?

[grimar]

tools/yaml2obj/yaml2elf.cpp

195 ↗

(On Diff #211025)

Perhaps having .strtab (even an empty one, i.e. only with a zero entry) is a bit more common?

I am not sure actually, but if you take an empty test.s and compile it with llvm-mc, you'll get a file which has:

Symbol table '.symtab' contains 1 entry:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND

[jhenderson]

tools/yaml2obj/yaml2elf.cpp

195 ↗

(On Diff #211025)

I guess it doesn't really matter either way.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct Section {
   // This can be used to override the sh_offset field. It does not place the
   // section data at the offset specified. Useful for creating invalid objects.
   Optional<llvm::yaml::Hex64> ShOffset;
 
   // This can be used to override the sh_size field. It does not affect the
   // content written.
   Optional<llvm::yaml::Hex64> ShSize;
 
-  Section(SectionKind Kind) : Kind(Kind) {}
+  // Usually sections are not created implicitly, but loaded from YAML.
+  // When they are, this flag is used to signal about that.
+  bool IsImplicit;
+
+  Section(SectionKind Kind, bool IsImplicit = false)
+      : Kind(Kind), IsImplicit(IsImplicit) {}
   virtual ~Section();
 };
 
 struct DynamicSection : Section {
   std::vector<DynamicEntry> Entries;
   Optional<yaml::BinaryRef> Content;
 
   DynamicSection() : Section(SectionKind::Dynamic) {}

llvm/tools/yaml2obj/yaml2elf.cpp

 //===----------------------------------------------------------------------===//
 
 #include "yaml2obj.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/ObjectYAML/ELFYAML.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
class ELFState {
   /// The future ".shstrtab" section.
   StringTableBuilder DotShStrtab{StringTableBuilder::ELF};
 
   /// The future ".dynstr" section.
   StringTableBuilder DotDynstr{StringTableBuilder::ELF};
 
   NameToIdxMap SN2I;
   NameToIdxMap SymN2I;
-  const ELFYAML::Object &Doc;
+  ELFYAML::Object &Doc;
 
   bool buildSectionIndex();
   bool buildSymbolIndex(ArrayRef<ELFYAML::Symbol> Symbols);
   void initELFHeader(Elf_Ehdr &Header);
   void initProgramHeaders(std::vector<Elf_Phdr> &PHeaders);
   bool initImplicitHeader(ELFState<ELFT> &State, ContiguousBlobAccumulator &CBA,
                           Elf_Shdr &Header, StringRef SecName,
                           ELFYAML::Section *YAMLSec);
bool writeSectionContent(Elf_Shdr &SHeader,
                            const ELFYAML::VerdefSection &Section,
                            ContiguousBlobAccumulator &CBA);
   bool writeSectionContent(Elf_Shdr &SHeader,
                            const ELFYAML::MipsABIFlags &Section,
                            ContiguousBlobAccumulator &CBA);
   bool writeSectionContent(Elf_Shdr &SHeader,
                            const ELFYAML::DynamicSection &Section,
                            ContiguousBlobAccumulator &CBA);
-  std::vector<StringRef> implicitSectionNames() const;
-
-  ELFState(const ELFYAML::Object &D) : Doc(D) {}
+  ELFState(ELFYAML::Object &D);
 
 public:
-  static int writeELF(raw_ostream &OS, const ELFYAML::Object &Doc);
+  static int writeELF(raw_ostream &OS, ELFYAML::Object &Doc);
 
 private:
   void finalizeStrings();
 };
 } // end anonymous namespace
 
 template <class ELFT>
+ELFState<ELFT>::ELFState(ELFYAML::Object &D) : Doc(D) {
+  StringSet<> DocSections;
+  for (std::unique_ptr<ELFYAML::Section> &D : Doc.Sections)
+    if (!D->Name.empty())
+      DocSections.insert(D->Name);
+
+  std::vector<StringRef> ImplicitSections = {".symtab", ".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 = llvm::make_unique<ELFYAML::Section>(
+        ELFYAML::Section::SectionKind::RawContent, true /*IsImplicit*/);
+    Sec->Name = SecName;
+    Doc.Sections.push_back(std::move(Sec));
+  }
+}
+
+template <class ELFT>
 void ELFState<ELFT>::initELFHeader(Elf_Ehdr &Header) {
   using namespace llvm::ELF;
   zero(Header);
   Header.e_ident[EI_MAG0] = 0x7f;
   Header.e_ident[EI_MAG1] = 'E';
   Header.e_ident[EI_MAG2] = 'L';
   Header.e_ident[EI_MAG3] = 'F';
   Header.e_ident[EI_CLASS] = ELFT::Is64Bits ? ELFCLASS64 : ELFCLASS32;
if (SuffixPos == StringRef::npos)
     return S;
   return S.substr(0, SuffixPos);
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::initSectionHeaders(ELFState<ELFT> &State,
                                         std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
-  // Build a list of sections we are going to add implicitly.
-  std::vector<StringRef> ImplicitSections;
-  for (StringRef Name : State.implicitSectionNames())
-    if (State.SN2I.get(Name) > Doc.Sections.size())
-      ImplicitSections.push_back(Name);
-
   // Ensure SHN_UNDEF entry is present. An all-zero section header is a
   // valid SHN_UNDEF entry since SHT_NULL == 0.
-  SHeaders.resize(Doc.Sections.size() + ImplicitSections.size() + 1);
+  SHeaders.resize(Doc.Sections.size() + 1);
   zero(SHeaders[0]);
 
-  for (size_t I = 1; I < Doc.Sections.size() + ImplicitSections.size() + 1; ++I) {
+  for (size_t I = 1; I < Doc.Sections.size() + 1; ++I) {
     Elf_Shdr &SHeader = SHeaders[I];
     zero(SHeader);
-    ELFYAML::Section *Sec =
-        I > Doc.Sections.size() ? nullptr : Doc.Sections[I - 1].get();
+    ELFYAML::Section *Sec = Doc.Sections[I - 1].get();
 
     // We have a few sections like string or symbol tables that are usually
     // added implicitly to the end. However, if they are explicitly specified
     // in the YAML, we need to write them here. This ensures the file offset
     // remains correct.
-    StringRef SecName =
-        Sec ? Sec->Name : ImplicitSections[I - Doc.Sections.size() - 1];
-    if (initImplicitHeader(State, CBA, SHeader, SecName, Sec))
+    if (initImplicitHeader(State, CBA, SHeader, Sec->Name,
+                           Sec->IsImplicit ? nullptr : Sec))
       continue;
 
     assert(Sec && "It can't be null unless it is an implicit section. But all "
                   "implicit sections should already have been handled above.");
 
-    SHeader.sh_name = DotShStrtab.getOffset(dropUniqueSuffix(SecName));
+    SHeader.sh_name = DotShStrtab.getOffset(dropUniqueSuffix(Sec->Name));
     SHeader.sh_type = Sec->Type;
     if (Sec->Flags)
       SHeader.sh_flags = *Sec->Flags;
     SHeader.sh_addr = Sec->Address;
     SHeader.sh_addralign = Sec->AddressAlign;
 
     if (!Sec->Link.empty()) {
       unsigned Index;
for (unsigned i = 0, e = Doc.Sections.size(); i != e; ++i) {
     // "+ 1" to take into account the SHT_NULL entry.
     if (!SN2I.addName(Name, i + 1)) {
       WithColor::error() << "Repeated section name: '" << Name
                          << "' at YAML section number " << i << ".\n";
       return false;
     }
   }
 
-  auto SecNo = 1 + Doc.Sections.size();
-  // Add special sections after input sections, if necessary.
-  for (StringRef Name : implicitSectionNames())
-    if (SN2I.addName(Name, SecNo)) {
-      // Account for this section, since it wasn't in the Doc
-      ++SecNo;
-      DotShStrtab.add(Name);
-    }
-
   DotShStrtab.finalize();
   return true;
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::buildSymbolIndex(ArrayRef<ELFYAML::Symbol> Symbols) {
   bool GlobalSymbolSeen = false;
   std::size_t I = 0;
if (auto VerNeed = dyn_cast<ELFYAML::VerneedSection>(Sec.get())) {
           DotDynstr.add(Name);
     }
   }
 
   DotDynstr.finalize();
 }
 
 template <class ELFT>
-int ELFState<ELFT>::writeELF(raw_ostream &OS, const ELFYAML::Object &Doc) {
+int ELFState<ELFT>::writeELF(raw_ostream &OS, ELFYAML::Object &Doc) {
   ELFState<ELFT> State(Doc);
 
   // Finalize .strtab and .dynstr sections. We do that early because want to
   // finalize the string table builders before writing the content of the
   // sections that might want to use them.
   State.finalizeStrings();
 
   if (!State.buildSectionIndex())
int ELFState<ELFT>::writeELF(raw_ostream &OS, ELFYAML::Object &Doc) {
 
   OS.write((const char *)&Header, sizeof(Header));
   writeArrayData(OS, makeArrayRef(PHeaders));
   writeArrayData(OS, makeArrayRef(SHeaders));
   CBA.writeBlobToStream(OS);
   return 0;
 }
 
-template <class ELFT>
-std::vector<StringRef> ELFState<ELFT>::implicitSectionNames() const {
-  if (Doc.DynamicSymbols.empty())
-    return {".symtab", ".strtab", ".shstrtab"};
-  return {".symtab", ".strtab", ".shstrtab", ".dynsym", ".dynstr"};
-}
-
 int yaml2elf(llvm::ELFYAML::Object &Doc, raw_ostream &Out) {
   bool IsLE = Doc.Header.Data == ELFYAML::ELF_ELFDATA(ELF::ELFDATA2LSB);
   bool Is64Bit = Doc.Header.Class == ELFYAML::ELF_ELFCLASS(ELF::ELFCLASS64);
   if (Is64Bit) {
     if (IsLE)
       return ELFState<object::ELF64LE>::writeELF(Out, Doc);
     return ELFState<object::ELF64BE>::writeELF(Out, Doc);
   }
   if (IsLE)
     return ELFState<object::ELF32LE>::writeELF(Out, Doc);
   return ELFState<object::ELF32BE>::writeELF(Out, Doc);
 }