[yaml2obj/obj2yaml] - Make fields of the `Symbol` to be optional.

[grimar]

Summary

This is similar to what we did earlier for fields of the Section class.

When a field is optional we can use the =<none> syntax in macros.
I am using this functionality in the patch that going to post soon.

I've added few test cases, I am not sure if we want to test all of fields,
as handling of "Optional<>" is generic and common.

[MaskRay]

llvm/tools/obj2yaml/elf2yaml.cpp

675

For undefined symbols, STB_GLOBAL is the default.
For defined symbols, STB_LOCAL is the default.

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

675

Before this patch our default binding was always ELF_STB (0), what is STB_LOCAL (0).

IO.mapOptional("Binding", Symbol.Binding, ELFYAML::ELF_STB(0));

I don't think that this patch is the right place to change defaults?

[MaskRay]

LGTM.

[jhenderson]

In general terms, I think we should have the following test cases for each field:

1. That a non-default value can be specified explicitly. If the value is an enum (like binding), each possible value should be specified.
2. That =<none> can be used where relevant, and that if it is, the value is whatever the default is.
3. That the field can be omitted entirely and the value is whatever the default is.

I don't think you need to test that =<none> values can be overridden. However, if you do override them, I wouldn't use the default value as the overriding value, personally. So, for example, with symbol binding, I'd override with STB_WEAK or STB_GLOBAL, rather than STB_LOCAL, to show that the default is not used still.

llvm/tools/obj2yaml/elf2yaml.cpp

668

Don't we need obj2yaml testing for these changes?

[grimar]

I'll split this and post a patch for each field instead.

llvm/include/llvm/ObjectYAML/ELFYAML.h

 
 struct SectionHeaderTable {
   Optional<std::vector<SectionHeader>> Sections;
   Optional<std::vector<SectionHeader>> Excluded;
   Optional<bool> NoHeaders;
 };
 
 struct Symbol {
-  StringRef Name;
-  ELF_STT Type;
-  StringRef Section;
+  Optional<StringRef> Name;
+  Optional<ELF_STT> Type;
+  Optional<StringRef> Section;
   Optional<ELF_SHN> Index;
-  ELF_STB Binding;
-  llvm::yaml::Hex64 Value;
-  llvm::yaml::Hex64 Size;
+  Optional<ELF_STB> Binding;
+  Optional<llvm::yaml::Hex64> Value;
+  Optional<llvm::yaml::Hex64> Size;
   Optional<uint8_t> Other;
 
   Optional<uint32_t> StName;
 };
 
 struct SectionOrType {
   StringRef sectionNameOrType;
 };

llvm/lib/ObjectYAML/ELFEmitter.cpp

void ELFState<ELFT>::assignSectionAddress(Elf_Shdr &SHeader,
 
   LocationCounter =
       alignTo(LocationCounter, SHeader.sh_addralign ? SHeader.sh_addralign : 1);
   SHeader.sh_addr = LocationCounter;
 }
 
 static size_t findFirstNonGlobal(ArrayRef<ELFYAML::Symbol> Symbols) {
   for (size_t I = 0; I < Symbols.size(); ++I)
-    if (Symbols[I].Binding.value != ELF::STB_LOCAL)
+    if (Symbols[I].Binding.getValueOr(ELF::STB_LOCAL) != ELF::STB_LOCAL)
       return I;
   return Symbols.size();
 }
 
 template <class ELFT>
 std::vector<typename ELFT::Sym>
 ELFState<ELFT>::toELFSymbols(ArrayRef<ELFYAML::Symbol> Symbols,
                              const StringTableBuilder &Strtab) {
   std::vector<Elf_Sym> Ret;
   Ret.resize(Symbols.size() + 1);
 
   size_t I = 0;
   for (const ELFYAML::Symbol &Sym : Symbols) {
     Elf_Sym &Symbol = Ret[++I];
 
     // If NameIndex, which contains the name offset, is explicitly specified, we
     // use it. This is useful for preparing broken objects. Otherwise, we add
     // the specified Name to the string table builder to get its offset.
     if (Sym.StName)
       Symbol.st_name = *Sym.StName;
-    else if (!Sym.Name.empty())
-      Symbol.st_name = Strtab.getOffset(ELFYAML::dropUniqueSuffix(Sym.Name));
+    else if (Sym.Name)
+      Symbol.st_name = Strtab.getOffset(ELFYAML::dropUniqueSuffix(*Sym.Name));
 
-    Symbol.setBindingAndType(Sym.Binding, Sym.Type);
-    if (!Sym.Section.empty())
-      Symbol.st_shndx = toSectionIndex(Sym.Section, "", Sym.Name);
+    Symbol.setBindingAndType(Sym.Binding.getValueOr(ELF::STB_LOCAL),
+                             Sym.Type.getValueOr(ELF::STT_NOTYPE));
+    if (Sym.Section)
+      Symbol.st_shndx =
+          toSectionIndex(*Sym.Section, "", Sym.Name.getValueOr(""));
     else if (Sym.Index)
       Symbol.st_shndx = *Sym.Index;
 
-    Symbol.st_value = Sym.Value;
-    Symbol.st_other = Sym.Other ? *Sym.Other : 0;
-    Symbol.st_size = Sym.Size;
+    Symbol.st_value = Sym.Value.getValueOr(yaml::Hex64(0));
+    Symbol.st_other = Sym.Other.getValueOr(yaml::Hex64(0));
+    Symbol.st_size = Sym.Size.getValueOr(yaml::Hex64(0));
   }
 
   return Ret;
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,
                                              SymtabType STType,
template <class ELFT> void ELFState<ELFT>::buildSectionIndex() {
 
   DotShStrtab.finalize();
 }
 
 template <class ELFT> void ELFState<ELFT>::buildSymbolIndexes() {
   auto Build = [this](ArrayRef<ELFYAML::Symbol> V, NameToIdxMap &Map) {
     for (size_t I = 0, S = V.size(); I < S; ++I) {
       const ELFYAML::Symbol &Sym = V[I];
-      if (!Sym.Name.empty() && !Map.addName(Sym.Name, I + 1))
-        reportError("repeated symbol name: '" + Sym.Name + "'");
+      if (Sym.Name && !Sym.Name->empty() && !Map.addName(*Sym.Name, I + 1))
+        reportError("repeated symbol name: '" + *Sym.Name + "'");
     }
   };
 
   if (Doc.Symbols)
     Build(*Doc.Symbols, SymN2I);
   if (Doc.DynamicSymbols)
     Build(*Doc.DynamicSymbols, DynSymN2I);
 }
 
 template <class ELFT> void ELFState<ELFT>::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
   if (Doc.Symbols)
     for (const ELFYAML::Symbol &Sym : *Doc.Symbols)
-      DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+      DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name.getValueOr("")));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
   if (Doc.DynamicSymbols)
     for (const ELFYAML::Symbol &Sym : *Doc.DynamicSymbols)
-      DotDynstr.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+      DotDynstr.add(ELFYAML::dropUniqueSuffix(Sym.Name.getValueOr("")));
 
   // SHT_GNU_verdef and SHT_GNU_verneed sections might also
   // add strings to .dynstr section.
   for (const ELFYAML::Chunk *Sec : Doc.getSections()) {
     if (auto VerNeed = dyn_cast<ELFYAML::VerneedSection>(Sec)) {
       if (VerNeed->VerneedV) {
         for (const ELFYAML::VerneedEntry &VE : *VerNeed->VerneedV) {
           DotDynstr.add(VE.File);

llvm/lib/ObjectYAML/ELFYAML.cpp

StringRef ScalarTraits<ELFYAML::YAMLIntUInt>::input(StringRef Scalar, void *Ctx,
   unsigned long long UInt;
   if (getAsUnsignedInteger(Scalar, /*Radix=*/0, UInt) || (UInt > MaxVal))
     return ErrMsg;
   Val = UInt;
   return "";
 }
 
 void MappingTraits<ELFYAML::Symbol>::mapping(IO &IO, ELFYAML::Symbol &Symbol) {
-  IO.mapOptional("Name", Symbol.Name, StringRef());
+  IO.mapOptional("Name", Symbol.Name);
   IO.mapOptional("StName", Symbol.StName);
-  IO.mapOptional("Type", Symbol.Type, ELFYAML::ELF_STT(0));
-  IO.mapOptional("Section", Symbol.Section, StringRef());
+  IO.mapOptional("Type", Symbol.Type);
+  IO.mapOptional("Section", Symbol.Section);
   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));
+  IO.mapOptional("Binding", Symbol.Binding);
+  IO.mapOptional("Value", Symbol.Value);
+  IO.mapOptional("Size", Symbol.Size);
 
   // Symbol's Other field is a bit special. It is usually a field that
   // represents st_other and holds the symbol visibility. However, on some
   // platforms, it can contain bit fields and regular values, or even sometimes a
   // crazy mix of them (see comments for NormalizedOther). Because of this, we
   // need special handling.
   MappingNormalization<NormalizedOther, Optional<uint8_t>> Keys(IO,
                                                                 Symbol.Other);
   IO.mapOptional("Other", Keys->Other);
 }
 
 std::string MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
                                                      ELFYAML::Symbol &Symbol) {
-  if (Symbol.Index && Symbol.Section.data())
+  if (Symbol.Index && Symbol.Section)
     return "Index and Section cannot both be specified for Symbol";
   return "";
 }
 
 static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {
   IO.mapOptional("Name", Section.Name, StringRef());
   IO.mapRequired("Type", Section.Type);
   IO.mapOptional("Flags", Section.Flags);

llvm/test/tools/yaml2obj/ELF/dynamic-symbols.yaml

 # SECTION-NEXT: SHF_ALLOC
 
 # SYMBOL-DAG: d dynlocal
 # SYMBOL-DAG: D dynglobal
 # SYMBOL-DAG: V dynweak
 
 ## Check we can use numeric values to refer to sections.
 
-# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: yaml2obj --docnum=2 -DSEC=0xff %s -o %t2
 # RUN: llvm-readobj --dyn-symbols %t2 2>&1 | FileCheck -DFILE=%t2 %s --check-prefix=NUM
 
 # NUM:      Name: foo
 # NUM:      Section:
 # NUM-SAME: .data (0x1)
 
 # NUM:      Name: bar
 # NUM:      Section:
 # NUM-SAME: .dynsym (0x2)
 
 # NUM:      Name: zed
 # NUM: warning: '[[FILE]]': invalid section index: 255
 # NUM:      Section:
 # NUM-SAME: <?> (0xFF)
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_DYN
 Sections:
   - Name: .data
     Type: SHT_PROGBITS
 DynamicSymbols:
-  - Name: foo
+  - Name:    foo
     Section: 1
-  - Name: bar
+  - Name:    bar
     Section: 2
-  - Name: zed
-    Section: 0xff
+  - Name:    zed
+    Section: [[SEC=<none>]]
+
+## Check that by default no section is set.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.none
+# RUN: llvm-readobj --dyn-symbols %t2.none 2>&1 | \
+# RUN:   FileCheck -DFILE=%t2.none %s --check-prefix=NONE
+
+# NONE:      Symbol {
+# NONE:        Name: zed
+# NONE-NOT:  }
+# NONE:        Section: Undefined (0x0)
+# NONE-NEXT: }
 
 ## Check we report errors when unknown sections are referenced by dynamic symbols.
 
 # RUN: not yaml2obj --docnum=3 %s 2>&1 | FileCheck -DFILE=%t3 %s --check-prefix=ERR
 
 # ERR: error: unknown section referenced: '.sec1' by YAML symbol 'foo'
 # ERR: error: unknown section referenced: '.sec2' by YAML symbol 'bar'
 

llvm/test/tools/yaml2obj/ELF/symbol-binding.yaml

 ## Check we can set different bindings for symbols.
 
-# RUN: yaml2obj %s | llvm-readobj --symbols - | FileCheck %s
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readobj --symbols %t1 | FileCheck %s
+
+## Check that STB_LOCAL is the default value for binding.
+
+# RUN: yaml2obj %s -DBINDING=STB_LOCAL -o %t2
+# RUN: cmp %t1 %t2
 
 !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Symbols:
   - Name:    local_symbol
     Type:    STT_OBJECT
-    Binding: STB_LOCAL
+    Binding: [[BINDING=<none>]]
   - Name:    global_symbol
     Type:    STT_OBJECT
     Binding: STB_GLOBAL
   - Name:    weak_symbol
     Type:    STT_OBJECT
     Binding: STB_WEAK
 
 # CHECK: Symbol {

llvm/test/tools/yaml2obj/ELF/symbol-index.yaml

-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readelf -s %t | FileCheck %s
+## Here we test that we can set the index of a symbol using the "Index" key.
+## Check that we set the index of a symbol to STN_UNDEF by default.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readelf -s %t1 | FileCheck %s
+
+# RUN: yaml2obj %s -DUNDEFINDEX=0 -o %t2
+# RUN: cmp %t1 %t2
 
 !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_EXEC
 Sections:
   - Name: .text
- Name:     good
     Binding:  STB_GLOBAL
   - Name:     bad
     Index:    0x42
     Binding:  STB_GLOBAL
   - Name:     undef1
     Index:    SHN_UNDEF
     Binding:  STB_GLOBAL
   - Name:     undef2
-    Index:    0
+    Index:    [[UNDEFINDEX=<none>]]
     Binding:  STB_GLOBAL
 
 # CHECK:      Symbol table '.symtab' contains 9 entries
 # CHECK-NEXT:   Num: {{.*}} Ndx Name
 # CHECK-NEXT:     0: {{.*}} UND
 # CHECK-NEXT:     1: {{.*}} ABS absolute1
 # CHECK-NEXT:     2: {{.*}} ABS absolute2
 # CHECK-NEXT:     3: {{.*}} COM common1
 # CHECK-NEXT:     4: {{.*}} COM common2
 # CHECK-NEXT:     5: {{.*}}   1 good
 # CHECK-NEXT:     6: {{.*}}  66 bad
 # CHECK-NEXT:     7: {{.*}} UND undef1
 # CHECK-NEXT:     8: {{.*}} UND undef2

llvm/test/tools/yaml2obj/ELF/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 -o %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)
+# CHECK: Name:   (0)
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Symbols:
   - Name:   test
   - StName: 1
   - StName: 2
   - Name:   1
   - Name:   2
+## Test that we can use '=<none>' syntax.
+## Test that we set sh_name to 0 by default.
+  - Name:   [[NAME=<none>]]
 
 ## Check we allow specifying both Name and StName at once.
 ## In this case StName has priority, but the symbol Name is still added
 ## to the symbol string table.
 
 # RUN: yaml2obj --docnum=2 %s -o %t2
 # RUN: llvm-readobj --symbols --sections --section-data -r --expand-relocs %t2 | FileCheck %s --check-prefix=BOTH
 

llvm/test/tools/yaml2obj/ELF/symbol-type.yaml

-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj --symbols %t | FileCheck %s
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readobj --symbols %t1 | FileCheck %s
+
+# RUN: yaml2obj %s -DTYPE=STT_NOTYPE -o %t2
+# RUN: cmp %t1 %t2
 
 # CHECK: Name: notype
 # CHECK: Type: None
 # CHECK: Name: normal_type
 # CHECK: Type: Object
 # CHECK: Name: .text
 # CHECK: Type: Section
 # CHECK: Name: known_hex
FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
 Symbols:
   - Name:    notype
-    Type:    STT_NOTYPE
+## Test that we can use '=<none>' syntax.
+## Test that we set the type to STT_NOTYPE by default.
+    Type:    [[TYPE=<none>]]
     Binding: STB_GLOBAL
   - Name:    normal_type
     Type:    STT_OBJECT
     Binding: STB_GLOBAL
   - Name:    .text
     Type:    STT_SECTION
     Binding: STB_GLOBAL
   - Name:    known_hex

llvm/tools/obj2yaml/elf2yaml.cpp

Error ELFDumper<ELFT>::dumpSymbols(const Elf_Shdr *Symtab,
   }
 
   return Error::success();
 }
 
 template <class ELFT>
 Error ELFDumper<ELFT>::dumpSymbol(const Elf_Sym *Sym, const Elf_Shdr *SymTab,
                                   StringRef StrTable, ELFYAML::Symbol &S) {
-  S.Type = Sym->getType();
-  S.Value = Sym->st_value;
-  S.Size = Sym->st_size;
+  if (Sym->getType() != ELF::STT_NOTYPE)
+    S.Type = (ELFYAML::ELF_STT)Sym->getType();
+  if (Sym->st_value)
+    S.Value = (yaml::Hex64)Sym->st_value;
+  if (Sym->st_size)
+    S.Size = (yaml::Hex64)Sym->st_size;
   S.Other = Sym->st_other;
+  if (Sym->getBinding() != ELF::STB_LOCAL)
     S.Binding = Sym->getBinding();
 
   Expected<StringRef> SymbolNameOrErr =
       getUniquedSymbolName(Sym, StrTable, SymTab);
   if (!SymbolNameOrErr)
     return SymbolNameOrErr.takeError();
   S.Name = SymbolNameOrErr.get();
 
   if (Sym->st_shndx >= ELF::SHN_LORESERVE) {