[yaml2obj] - Allow setting st_value explicitly for Symbol.

[grimar]

Summary

In some cases it is useful to explicitly set symbol's st_name value.
For example, I am using it in a patch for LLD to remove the broken
binary from a test case and replace it with a YAML test.

[jhenderson]

I have a slight concern here, in that the approach prevents us using a name that is actually a number, and that might be confusing. I think we might want to have a different field (e.g. "NameIndex" or similar) explicitly for this situation. One and only one of the two fields can then be specified.

What do you think?

[grimar]

In D61180#1482531, @jhenderson wrote:

I have a slight concern here, in that the approach prevents us using a name that is actually a number, and that might be confusing. I think we might want to have a different field (e.g. "NameIndex" or similar) explicitly for this situation. One and only one of the two fields can then be specified.

What do you think?

I thought that it is impossible to have a symbol name that is a number. Because valid C name must begin with a letter of the alphabet or an underscore.
I.e. I am not sure we should care about this case. At least now. Should we?

[jhenderson]

In D61180#1482568, @grimar wrote:

I thought that it is impossible to have a symbol name that is a number. Because valid C name must begin with a letter of the alphabet or an underscore.
I.e. I am not sure we should care about this case. At least now. Should we?

Maybe not in C, but there's nothing in ELF that requires this, and I believe it might be possible to name them like that in assembly using double quotes.

[grimar]

Ok. Planning changes. Will take some time since I will be off starting from tomorrow by EOW because of holidays here.

[grimar]

• Reimplemented as was suggested during the review.

[jhenderson]

Some comment nits, otherwise LGTM.

test/tools/yaml2obj/symbol-name.yaml

1

FWIW, I liked what you did in other tests with the double '#' symbols to indicate comments. I wouldn't object to you doing it here either.

tools/yaml2obj/yaml2elf.cpp

472

name index containing offset -> NameIndex, which contains the name offset,
remove "in YAML", since theoretically, we could use this bit of code via other methods in the future.

473

It is -> This is
a Name -> the specified Name

include/llvm/ObjectYAML/ELFYAML.h

struct ProgramHeader {
   Optional<llvm::yaml::Hex64> FileSize;
   Optional<llvm::yaml::Hex64> MemSize;
   Optional<llvm::yaml::Hex64> Offset;
   std::vector<SectionName> Sections;
 };
 
 struct Symbol {
   StringRef Name;
+  Optional<uint32_t> NameIndex;
   ELF_STT Type;
   StringRef Section;
   Optional<ELF_SHN> Index;
   ELF_STB Binding;
   llvm::yaml::Hex64 Value;
   llvm::yaml::Hex64 Size;
   uint8_t Other;
 };

lib/ObjectYAML/ELFYAML.cpp

struct NormalizedOther {
   ELFYAML::ELF_STV Visibility;
   ELFYAML::ELF_STO Other;
 };
 
 } // end anonymous namespace
 
 void MappingTraits<ELFYAML::Symbol>::mapping(IO &IO, ELFYAML::Symbol &Symbol) {
   IO.mapOptional("Name", Symbol.Name, StringRef());
+  IO.mapOptional("NameIndex", Symbol.NameIndex);
   IO.mapOptional("Type", Symbol.Type, ELFYAML::ELF_STT(0));
   IO.mapOptional("Section", Symbol.Section, StringRef());
   IO.mapOptional("Index", Symbol.Index);
   IO.mapOptional("Binding", Symbol.Binding, ELFYAML::ELF_STB(0));
   IO.mapOptional("Value", Symbol.Value, Hex64(0));
   IO.mapOptional("Size", Symbol.Size, Hex64(0));
   MappingNormalization<NormalizedOther, uint8_t> Keys(IO, Symbol.Other);
   IO.mapOptional("Visibility", Keys->Visibility, ELFYAML::ELF_STV(0));
   IO.mapOptional("Other", Keys->Other, ELFYAML::ELF_STO(0));
 }
 
 StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
                                                    ELFYAML::Symbol &Symbol) {
-  if (Symbol.Index && Symbol.Section.data()) {
+  if (Symbol.Index && Symbol.Section.data())
     return "Index and Section cannot both be specified for Symbol";
-  }
-  if (Symbol.Index && *Symbol.Index == ELFYAML::ELF_SHN(ELF::SHN_XINDEX)) {
+  if (Symbol.Index && *Symbol.Index == ELFYAML::ELF_SHN(ELF::SHN_XINDEX))
     return "Large indexes are not supported";
-  }
+  if (Symbol.NameIndex && !Symbol.Name.empty())
+    return "Name and NameIndex cannot both be specified for Symbol";
   return StringRef();
 }
 
 static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {
   IO.mapOptional("Name", Section.Name, StringRef());
   IO.mapRequired("Type", Section.Type);
   IO.mapOptional("Flags", Section.Flags, ELFYAML::ELF_SHF(0));
   IO.mapOptional("Address", Section.Address, Hex64(0));

test/tools/yaml2obj/symbol-name.yaml

+# Check we are able to use integers as both
+# symbol name indices (st_name values) and symbol names.
+# RUN: yaml2obj --docnum=1 %s > %t
+# RUN: llvm-readobj --symbols %t | FileCheck %s
+
+# CHECK: Name: test (1)
+# CHECK: Name: test (1)
+# CHECK: Name: est (2)
+# CHECK: Name: 1 (8)
+# CHECK: Name: 2 (6)
+
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+Symbols:
+  - Name:      test
+  - NameIndex: 1
+  - NameIndex: 2
+  - Name:      1
+  - Name:      2
+
+# Check we do not allow specifying both Name and NameIndex at once.
+# RUN: not yaml2obj --docnum=2 %s 2>&1 | FileCheck %s --check-prefix=ERR
+# ERR: error: Name and NameIndex cannot both be specified for Symbol
+
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+Symbols:
+  - Name:      foo
+    NameIndex: 0

tools/yaml2obj/yaml2elf.cpp

raw_ostream &getOSAndAlignedOffset(Integer &Offset, unsigned Align) {
     Offset = padToAlignment(Align);
     return OS;
   }
   void writeBlobToStream(raw_ostream &Out) { Out << OS.str(); }
 };
 } // end anonymous namespace
 
 // Used to keep track of section and symbol names, so that in the YAML file
-// sections and symbols can be referenced by name instead of by index.
-namespace {
-class NameToIdxMap {
+// sections and symbols can be referenced by name instead of by index.
+namespace {
+class NameToIdxMap {
   StringMap<unsigned> Map;
 
-public:
+public:
   /// \Returns false if name is already present in the map.
   bool addName(StringRef Name, unsigned Ndx) {
     return Map.insert({Name, Ndx}).second;
-  }
+  }
   /// \Returns false if name is not present in the map.
-  bool lookup(StringRef Name, unsigned &Idx) const {
+  bool lookup(StringRef Name, unsigned &Idx) const {
     auto I = Map.find(Name);
-    if (I == Map.end())
+    if (I == Map.end())
       return false;
-    Idx = I->getValue();
+    Idx = I->getValue();
     return true;
-  }
+  }
   /// Asserts if name is not present in the map.
-  unsigned get(StringRef Name) const {
+  unsigned get(StringRef Name) const {
     unsigned Idx;
     if (lookup(Name, Idx))
       return Idx;
     assert(false && "Expected section not found in index");
     return 0;
-  }
-  unsigned size() const { return Map.size(); }
-};
+  }
+  unsigned size() const { return Map.size(); }
+};
 } // end anonymous namespace
 
 template <class T>
 static size_t arrayDataSize(ArrayRef<T> A) {
   return A.size() * sizeof(T);
 }
 
 template <class T>
for (const auto &YamlPhdr : Doc.ProgramHeaders) {
     Elf_Phdr Phdr;
     Phdr.p_type = YamlPhdr.Type;
     Phdr.p_flags = YamlPhdr.Flags;
     Phdr.p_vaddr = YamlPhdr.VAddr;
     Phdr.p_paddr = YamlPhdr.PAddr;
     PHeaders.push_back(Phdr);
   }
 }
-
-static bool convertSectionIndex(NameToIdxMap &SN2I, StringRef SecName,
-                                StringRef IndexSrc, unsigned &IndexDest) {
+
+static bool convertSectionIndex(NameToIdxMap &SN2I, StringRef SecName,
+                                StringRef IndexSrc, unsigned &IndexDest) {
   if (!SN2I.lookup(IndexSrc, IndexDest) && !to_integer(IndexSrc, IndexDest)) {
-    WithColor::error() << "Unknown section referenced: '" << IndexSrc
-                       << "' at YAML section '" << SecName << "'.\n";
-    return false;
+    WithColor::error() << "Unknown section referenced: '" << IndexSrc
+                       << "' at YAML section '" << SecName << "'.\n";
+    return false;
   }
   return true;
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
   // Ensure SHN_UNDEF entry is present. An all-zero section header is a

 
 template <class ELFT>
 void ELFState<ELFT>::addSymbols(ArrayRef<ELFYAML::Symbol> Symbols,
                                 std::vector<Elf_Sym> &Syms,
                                 const StringTableBuilder &Strtab) {
   for (const auto &Sym : Symbols) {
     Elf_Sym Symbol;
     zero(Symbol);
-    if (!Sym.Name.empty())
+
+    // If name index containing offset is explicitly specified in YAML, we use
+    // it. It is useful for preparing broken objects. Otherwise, we add a Name
+    // to the string table builder to get its offset.
+    if (Sym.NameIndex)
+      Symbol.st_name = *Sym.NameIndex;
+    else if (!Sym.Name.empty())
       Symbol.st_name = Strtab.getOffset(Sym.Name);
-    Symbol.setBindingAndType(Sym.Binding, Sym.Type);
-    if (!Sym.Section.empty()) {
-      unsigned Index;
+
+    Symbol.setBindingAndType(Sym.Binding, Sym.Type);
+    if (!Sym.Section.empty()) {
+      unsigned Index;
       if (!SN2I.lookup(Sym.Section, Index)) {
-        WithColor::error() << "Unknown section referenced: '" << Sym.Section
-                           << "' by YAML symbol " << Sym.Name << ".\n";
-        exit(1);
+        WithColor::error() << "Unknown section referenced: '" << Sym.Section
+                           << "' by YAML symbol " << Sym.Name << ".\n";
+        exit(1);
       }
       Symbol.st_shndx = Index;
     } else if (Sym.Index) {
       Symbol.st_shndx = *Sym.Index;
     }
     // else Symbol.st_shndex == SHN_UNDEF (== 0), since it was zero'd earlier.
     Symbol.st_value = Sym.Value;
     Symbol.st_other = Sym.Other;
ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   if (!Section.RelocatableSec.empty() &&
       !convertSectionIndex(SN2I, Section.Name, Section.RelocatableSec, Index))
     return false;
   SHeader.sh_info = Index;
 
   auto &OS = CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
 
   for (const auto &Rel : Section.Relocations) {
-    unsigned SymIdx = 0;
-    // If a relocation references a symbol, try to look one up in the symbol
-    // table. If it is not there, treat the value as a symbol index.
+    unsigned SymIdx = 0;
+    // If a relocation references a symbol, try to look one up in the symbol
+    // table. If it is not there, treat the value as a symbol index.
     if (Rel.Symbol && !SymN2I.lookup(*Rel.Symbol, SymIdx) &&
-        !to_integer(*Rel.Symbol, SymIdx)) {
-      WithColor::error() << "Unknown symbol referenced: '" << *Rel.Symbol
-                         << "' at YAML section '" << Section.Name << "'.\n";
+        !to_integer(*Rel.Symbol, SymIdx)) {
+      WithColor::error() << "Unknown symbol referenced: '" << *Rel.Symbol
+                         << "' at YAML section '" << Section.Name << "'.\n";
       return false;
     }
 
     if (IsRela) {
       Elf_Rela REntry;
       zero(REntry);
       REntry.r_offset = Rel.Offset;
       REntry.r_addend = Rel.Addend;

 template <class ELFT>
 bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
                                          const ELFYAML::Group &Section,
                                          ContiguousBlobAccumulator &CBA) {
   assert(Section.Type == llvm::ELF::SHT_GROUP &&
          "Section type is not SHT_GROUP");
 
   SHeader.sh_entsize = 4;
-  SHeader.sh_size = SHeader.sh_entsize * Section.Members.size();
-
+  SHeader.sh_size = SHeader.sh_entsize * Section.Members.size();
+
   unsigned SymIdx;
   if (!SymN2I.lookup(Section.Signature, SymIdx) &&
       !to_integer(Section.Signature, SymIdx)) {
     WithColor::error() << "Unknown symbol referenced: '" << Section.Signature
                        << "' at YAML section '" << Section.Name << "'.\n";
     return false;
   }
   SHeader.sh_info = SymIdx;
bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   if (Section.Content)
     Section.Content->writeAsBinary(OS);
 
   return true;
 }
 
 template <class ELFT> bool ELFState<ELFT>::buildSectionIndex() {
   for (unsigned i = 0, e = Doc.Sections.size(); i != e; ++i) {
-    StringRef Name = Doc.Sections[i]->Name;
-    DotShStrtab.add(Name);
-    // "+ 1" to take into account the SHT_NULL entry.
+    StringRef Name = Doc.Sections[i]->Name;
+    DotShStrtab.add(Name);
+    // "+ 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;
+      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
for (const auto &Sym : Symbols) {
     ++I;
 
     StringRef Name = Sym.Name;
     if (Sym.Binding.value == ELF::STB_LOCAL && GlobalSymbolSeen) {
       WithColor::error() << "Local symbol '" + Name +
                                 "' after global in Symbols list.\n";
       return false;
     }
-    if (Sym.Binding.value != ELF::STB_LOCAL)
-      GlobalSymbolSeen = true;
-
+    if (Sym.Binding.value != ELF::STB_LOCAL)
+      GlobalSymbolSeen = true;
+
     if (!Name.empty() && !SymN2I.addName(Name, I)) {
-      WithColor::error() << "Repeated symbol name: '" << Name << "'.\n";
-      return false;
-    }
+      WithColor::error() << "Repeated symbol name: '" << Name << "'.\n";
+      return false;
+    }
   }
   return true;
 }
 
 template <class ELFT> void ELFState<ELFT>::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
   for (const ELFYAML::Symbol &Sym : Doc.Symbols)
     DotStrtab.add(Sym.Name);