[yaml2obj] - Add a technical suffix for each unnamed chunk.

[grimar]

Summary

This change does not affect the produced binary.

In this patch I assign a technical suffix to each section/fill
(i.e. chunk) name when it is empty. It allows to simplify the code
slightly and improve error messages reported.

In the code we have the section to index mapping, SN2I, which is
globally used. With this change we can use it to map "empty"
names to indexes now, what is helpful.

[grimar]

D79985 depends on this, but it can live without it. I am going to post another patch soon that is using SN2I and requires this change.

[MaskRay]

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

Can we omit unnamed and use '' (index 2)?

[grimar]

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

We can omit/add whatever we want into [ ... ] part.

This patch works because it automatically adds unique suffixes. What we use for
describing sections/symbols with the same name already:

- Name: '.foo [1]'
  Type: SHT_PROGBITS
- Name: '.foo [2]'
  Type: SHT_PROGBITS

I.e. we can add any [ any text here ] as suffix to a section name and the existent logic will
handle it properly. But we can't switch [] to () without doing additional changes.

It could be ' [index #2]', but not '' [index 2] nor '' (index 2).

It can be possible to change (add an additional name parsing of a section name) in
toSectionIndex to improve the message reported, but I am not sure it worth that additional complexity:

template <class ELFT>
unsigned ELFState<ELFT>::toSectionIndex(StringRef S, StringRef LocSec,
                                        StringRef LocSym) {
....
//// Add a code to parse LocSec/LocSym here.

  if (!LocSym.empty())
    reportError("unknown section referenced: '" + S + "' by YAML symbol '" +
                LocSym + "'");
  else
    reportError("unknown section referenced: '" + S + "' by YAML section '" +
                LocSec + "'");
  return 0;
}

[MaskRay]

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

OK, this name should be fine. But why doesn't '' [index 2]" work? (I am actually ok with either one but #2 not working seems strange to me.)

[grimar]

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

See.

1. We have the following code:

StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
  size_t SuffixPos = S.rfind(" [");
  if (SuffixPos == StringRef::npos)
    return S;
  return S.substr(0, SuffixPos);
}

I.e. we can safely add [ suffix and everything starting from it will be ignored.
The empty section name with a suffix looks like " [...", where ... is any data or nothing.
e.g " [ index 2 ]"/" [ foo"/" [" are valid empty section names.

2. The code that reports an error is:

reportError("unknown section referenced: '" + S + "' by YAML section '" +
                LocSec + "'");

I.e. if the section name is X, it prints:

unknown section referenced: '.foo' by YAML section 'X'

How can it print '' [index 2]? For section name " [ index 2 ]" it would print:

unknown section referenced: '.foo' by YAML section ' [index 2]`

[jhenderson]

llvm/lib/ObjectYAML/ELFEmitter.cpp

252

You refer to the name here as a prefix, but it's really a suffix. I think calling it a prefix is slightly confusing given it is supposed to be equivalent to the unique suffix used for identically-named sections.

Perhaps also worth mentioning that this should be identical style to the seciton name uniquing. In fact, if there's a way to enforce that identical property by code sharing, that would be even better.

252–254

Minor nit: comment flowing here seems a little weird (I'd expect "report" to be on the line before at least).

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

I can probably live with the bit in square brackets being inside the quotes, but is it possible to get rid of the extra space at least? That might require a little extra logic in dropUniqueSuffix to work, but I think it might be a nice property, and might even be more generally useful (allowing slight variations in whitespace in unique names without affecting behaviour).

[grimar]

• Addressed review comments.

Note: 2 test cases are failing with this change. See my related question inlined.

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

but is it possible to get rid of the extra space at least?

It is problematic because we have at least 2 tests that doesn't like it:
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test#L111
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/macro.yaml#L57

The best solution I see is to start using "<>" brackets instead of "[]" for unique suffixes.

I made a code change in this patch to show how it might work. So what about if I change []-><> first?

[grimar]

llvm/test/tools/yaml2obj/ELF/section-link.yaml

32

I've changed "[]" to "()" here: D80123

[grimar]

• Updated.

Ready for review.

[MaskRay]

llvm/lib/ObjectYAML/ELFEmitter.cpp

415

I think the code can be simplified if you change here to constexpr char SuffixStart = '('

420

s/a //

[jhenderson]

LGTM, once @MaskRay's comments have been addressed.

llvm/lib/ObjectYAML/ELFEmitter.cpp

420

We definitely want the first "a". I think the second one is acceptable either way, so I'd say either leave as-is or "Do not add a space when Name is empty."

[grimar]

• Addressed review comments.

llvm/lib/ObjectYAML/ELFEmitter.cpp

415

Done.

[grimar]

@MaskRay, are you happy with the code change?

llvm/lib/ObjectYAML/ELFEmitter.cpp

void writeSectionContent(Elf_Shdr &SHeader,
                            ContiguousBlobAccumulator &CBA);
 
   void writeFill(ELFYAML::Fill &Fill, ContiguousBlobAccumulator &CBA);
 
   ELFState(ELFYAML::Object &D, yaml::ErrorHandler EH);
 
   void assignSectionAddress(Elf_Shdr &SHeader, ELFYAML::Section *YAMLSec);
 
+  BumpPtrAllocator StringAlloc;
   uint64_t alignToOffset(ContiguousBlobAccumulator &CBA, uint64_t Align,
                          llvm::Optional<llvm::yaml::Hex64> Offset);
 
 public:
   static bool writeELF(raw_ostream &OS, ELFYAML::Object &Doc,
                        yaml::ErrorHandler EH);
 };
 } // end anonymous namespace
 
 template <class T> static size_t arrayDataSize(ArrayRef<T> A) {
   return A.size() * sizeof(T);
 }
 
 template <class T> static void writeArrayData(raw_ostream &OS, ArrayRef<T> A) {
   OS.write((const char *)A.data(), arrayDataSize(A));
 }
 
 template <class T> static void zero(T &Obj) { memset(&Obj, 0, sizeof(Obj)); }
 
 template <class ELFT>
 ELFState<ELFT>::ELFState(ELFYAML::Object &D, yaml::ErrorHandler EH)
     : Doc(D), ErrHandler(EH) {
   std::vector<ELFYAML::Section *> Sections = Doc.getSections();
-  StringSet<> DocSections;
-  for (const ELFYAML::Section *Sec : Sections)
-    if (!Sec->Name.empty())
-      DocSections.insert(Sec->Name);
-
   // Insert SHT_NULL section implicitly when it is not defined in YAML.
   if (Sections.empty() || Sections.front()->Type != ELF::SHT_NULL)
     Doc.Chunks.insert(
         Doc.Chunks.begin(),
         std::make_unique<ELFYAML::Section>(
             ELFYAML::Chunk::ChunkKind::RawContent, /*IsImplicit=*/true));
 
+  // We add a technical suffix for each unnamed section/fill. It does not affect
+  // the output, but allows us to map them by name in the code and report better
+  // error messages.
+  StringSet<> DocSections;
+  for (size_t I = 0; I < Doc.Chunks.size(); ++I) {
+    const std::unique_ptr<ELFYAML::Chunk> &C = Doc.Chunks[I];
+    if (C->Name.empty()) {
+      std::string NewName = ELFYAML::appendUniqueSuffix(
+          /*Name=*/"", "index " + Twine(I));
+      C->Name = StringRef(NewName).copy(StringAlloc);
+      assert(ELFYAML::dropUniqueSuffix(C->Name).empty());
+    }
+    DocSections.insert(C->Name);
+  }
+
   std::vector<StringRef> ImplicitSections;
   if (Doc.DynamicSymbols)
     ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
   if (Doc.Symbols)
     ImplicitSections.push_back(".symtab");
   ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});
 
   // Insert placeholders for implicit sections that are not
bool ELFState<ELFT>::initImplicitHeader(ContiguousBlobAccumulator &CBA,
 
   LocationCounter += Header.sh_size;
 
   // Override section fields if requested.
   overrideFields<ELFT>(YAMLSec, Header);
   return true;
 }
 
-constexpr StringRef SuffixStart = " (";
+constexpr char SuffixStart = '(';
 constexpr char SuffixEnd = ')';
 
 std::string llvm::ELFYAML::appendUniqueSuffix(StringRef Name,
                                               const Twine &Msg) {
-  return (Name + SuffixStart + Msg + Twine(SuffixEnd)).str();
+  // Do not add a space when a Name is empty.
+  std::string Ret = Name.empty() ? "" : Name.str() + ' ';
+  return Ret + (Twine(SuffixStart) + Msg + Twine(SuffixEnd)).str();
 }
 
 StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
   if (S.empty() || S.back() != SuffixEnd)
     return S;
 
+  // A special case for empty names. See appendUniqueSuffix() above.
   size_t SuffixPos = S.rfind(SuffixStart);
-  if (SuffixPos == StringRef::npos)
+  if (SuffixPos == 0)
+    return "";
+
+  if (SuffixPos == StringRef::npos || S[SuffixPos - 1] != ' ')
     return S;
-  return S.substr(0, SuffixPos);
+  return S.substr(0, SuffixPos - 1);
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
   // Ensure SHN_UNDEF entry is present. An all-zero section header is a
   // valid SHN_UNDEF entry since SHT_NULL == 0.
   SHeaders.resize(Doc.getSections().size());
 
-  size_t SecNdx = -1;
   for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     if (ELFYAML::Fill *S = dyn_cast<ELFYAML::Fill>(D.get())) {
       S->Offset = alignToOffset(CBA, /*Align=*/1, S->Offset);
       writeFill(*S, CBA);
       LocationCounter += S->Size;
       continue;
     }
 
-    ++SecNdx;
     ELFYAML::Section *Sec = cast<ELFYAML::Section>(D.get());
-    if (SecNdx == 0 && Sec->IsImplicit)
+    bool IsFirstUndefSection = D == Doc.Chunks.front();
+    if (IsFirstUndefSection && Sec->IsImplicit)
       continue;
 
     // 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.
-    Elf_Shdr &SHeader = SHeaders[SecNdx];
+    Elf_Shdr &SHeader = SHeaders[SN2I.get(Sec->Name)];
     if (initImplicitHeader(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(ELFYAML::dropUniqueSuffix(Sec->Name));
     SHeader.sh_type = Sec->Type;
     if (Sec->Flags)
       SHeader.sh_flags = *Sec->Flags;
     SHeader.sh_addralign = Sec->AddressAlign;
 
     // Set the offset for all sections, except the SHN_UNDEF section with index
     // 0 when not explicitly requested.
-    bool IsFirstUndefSection = SecNdx == 0;
     if (!IsFirstUndefSection || Sec->Offset)
       SHeader.sh_offset = alignToOffset(CBA, SHeader.sh_addralign, Sec->Offset);
 
     assignSectionAddress(SHeader, Sec);
 
     if (!Sec->Link.empty())
       SHeader.sh_link = toSectionIndex(Sec->Link, Sec->Name);
 
template <class ELFT> void ELFState<ELFT>::buildSectionIndex() {
   size_t SecNdx = -1;
   StringSet<> Seen;
   for (size_t I = 0; I < Doc.Chunks.size(); ++I) {
     const std::unique_ptr<ELFYAML::Chunk> &C = Doc.Chunks[I];
     bool IsSection = isa<ELFYAML::Section>(C.get());
     if (IsSection)
       ++SecNdx;
 
-    if (C->Name.empty())
-      continue;
-
     if (!Seen.insert(C->Name).second)
       reportError("repeated section/fill name: '" + C->Name +
                   "' at YAML section/fill number " + Twine(I));
     if (!IsSection || HasError)
       continue;
 
     if (!SN2I.addName(C->Name, SecNdx))
       llvm_unreachable("buildSectionIndex() failed");

llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml

- Type:         SHT_NULL
     Link:         0
   - Type: SHT_PROGBITS
     Name: 'foo'
 
 ## Check we report an error if null section sh_link field refers to an unknown section.
 
 # RUN: not yaml2obj --docnum=6 %s -o %t6 2>&1 | FileCheck %s --check-prefix=CASE4
 
-# CASE4: error: unknown section referenced: '.foo' by YAML section ''
+# CASE4:      error: unknown section referenced: '.foo' by YAML section '(index 0)'
+# CASE4-NEXT: error: unknown section referenced: '.bar' by YAML section '(index 1)'
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Type: SHT_NULL
     Link: .foo
+  - Type: SHT_NULL
+    Link: .bar
 
 ## Check that null section fields are set to zero, if they are unspecified.
 
 # RUN: yaml2obj --docnum=7 %s -o %t7
 # RUN: llvm-readelf --sections %t7 | FileCheck %s --check-prefix=DEFAULT
 
 --- !ELF
 FileHeader:

llvm/test/tools/yaml2obj/ELF/section-link.yaml

- Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     Link:            12345
 
 ## Check we report an error when an unknown section is referenced via a Link field.
 
 # RUN: not yaml2obj --docnum=2 %s 2>&1 | FileCheck %s --check-prefix=ERR
 
-# ERR: error: unknown section referenced: '.unknown1' by YAML section '.foo'
-# ERR: error: unknown section referenced: '.unknown2' by YAML section '.bar'
+# ERR:      error: unknown section referenced: '.unknown1' by YAML section '.foo'
+# ERR-NEXT: error: unknown section referenced: '.unknown2' by YAML section '(index 2)'
+# ERR-NEXT: error: unknown section referenced: '.unknown3' by YAML section '.bar'
+# ERR-NEXT: error: unknown section referenced: '.unknown4' by YAML section '(index 4)'
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .foo
     Type: SHT_PROGBITS
     Link: .unknown1
+  - Type: SHT_PROGBITS
+    Link: .unknown2
   - Name: .bar
     Type: SHT_PROGBITS
-    Link: .unknown2
+    Link: .unknown3
+  - Type: SHT_PROGBITS
+    Link: .unknown4
 
 ## Check we link SHT_GROUP to a symbol table by default if it exists.
 ## Also, check we can set an arbitrary value for sh_link.
 
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-readobj --sections %t3 | FileCheck %s --check-prefix=GROUP-LINK
 
 # GROUP-LINK:      Name: .group1