[yaml2obj] - Do not assert when .dynsym is specified explicitly, but .dynstr is not present.

[grimar]

Summary

We have a code in buildSectionIndex() that adds implicit sections:

// 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);
  }

The problem arises when .dynsym is specified explicitly and no
DynamicSymbols is used. In that case, we do not add
.dynstr implicitly and will assert later when will try to set Link
for .dynsym.

Seems, in this case, reasonable behavior is to allow Link field to be zero.
This is what this patch does.

[grimar]

It is rebased on top of D63002.

[jhenderson]

test/tools/yaml2obj/explicit-dynsym-no-dynstr.yaml

7–9 ↗

(On Diff #203524)

Could you file/find a bug for this and include the bug URL here?

Also report -> reports

tools/yaml2obj/yaml2elf.cpp

371 ↗

(On Diff #203524)

in document -> in the document

372 ↗

(On Diff #203524)

omit "DynamicSymbols" -> omit the "DynamicSymbols"
You don't need the ',' after "case"

373 ↗

(On Diff #203524)

described -> defined?

375–379 ↗

(On Diff #203524)

It looks like this doesn't allow for a custom-specified sh_link field. It might be nice to add that later.

[grimar]

• Addressed review comments.

tools/yaml2obj/yaml2elf.cpp

375–379 ↗

(On Diff #203524)

Yes, it will be needed to make a complete test case for https://bugs.llvm.org/show_bug.cgi?id=42215

[jhenderson]

LGTM.

llvm/trunk/test/tools/yaml2obj/explicit-dynsym-no-dynstr.yaml

+## Check we do not crash/assert when .dynsym is specified
+## explicitly, but .dynstr is not present.
+
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-readelf --section-headers %t 2>&1 | FileCheck %s
+
+## TODO: Check that .dynsym has Link field set to 0.
+##       GNU readelf is able to dump sections headers,
+##       but llvm-readelf reports an error below too early.
+##       See https://bugs.llvm.org/show_bug.cgi?id=42215.
+
+# CHECK: error: invalid sh_type for string table, expected SHT_STRTAB
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynsym
+    Type: SHT_SYMTAB

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,
                                              SymtabType STType,
                                              ContiguousBlobAccumulator &CBA,
                                              ELFYAML::Section *YAMLSec) {
   zero(SHeader);
   bool IsStatic = STType == SymtabType::Static;
   SHeader.sh_name = DotShStrtab.getOffset(IsStatic ? ".symtab" : ".dynsym");
   SHeader.sh_type = IsStatic ? ELF::SHT_SYMTAB : ELF::SHT_DYNSYM;
-  SHeader.sh_link = IsStatic ? SN2I.get(".strtab") : SN2I.get(".dynstr");
+
+  // When we describe the .dynsym section in the document explicitly, it is
+  // allowed to omit the "DynamicSymbols" tag. In this case .dynstr is not added
+  // implicitly and we should be able to leave the Link zeroed if .dynstr is not
+  // defined.
+  unsigned Link = 0;
+  if (IsStatic)
+    Link = SN2I.get(".strtab");
+  else
+    SN2I.lookup(".dynstr", Link);
+  SHeader.sh_link = Link;
+
   if (!IsStatic)
     SHeader.sh_flags |= ELF::SHF_ALLOC;
 
   // One greater than symbol table index of the last local symbol.
   const auto &Symbols = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
 
   // If the symbol table section is explicitly described in the YAML
   // then we should set the fields requested.