[yaml2obj/obj2yaml] - Make Value/Size fields of Symbol optional.

[grimar]

Summary

When a field is optional we can use the =<none> syntax in macros.
This patch makes Value/Size fields of Symbol optional
and adds test cases for them.

[jhenderson]

llvm/test/tools/obj2yaml/ELF/symbol.yaml

1

16

That's needed or you won't be able to add more documents in the future to the test file, I've discovered. Same below.

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

1

I'm slightly concerned that this test name and purpose is a bit restrictive. Why doesn't it apply to other symbol fields, like type, other or section index? (I'm not saying those tests need adding in this change, but it feels like there's no special reason why value and size are in one test and the others aren't in the same one).

3

Perhaps worth a comment saying something like "show the default size and value is 0".

16–17

30

36

[grimar]

• Addressed review comments.

llvm/test/tools/obj2yaml/ELF/symbol.yaml

16

You actually can add documents, you just need to be sure that 2nd and other document has --- !ELF.
For the first one it is not really important. I did it though.

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

1

I did it for consistency. Currently we have the following list of tests for symbols:

symbol-stother.yaml
symbol-type.yaml
symbol-binding.yaml
symbol-index.yaml
symbol-index-invalid.yaml
symbol-name.yaml
symbol-visibility.yaml

Perhaps, we should merge them into a single test in a follow-up.

3

The symbol to "show the default size and value is 0" is aaa, I have the comment for it below.
This particular call and check lines are not not testing anything valuable (used to bypass
the header and the null symbol).

[grimar]

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

1

Note that value and size are the only llvm::yaml::Hex64 values of a symbol.
And the logic of handling them is very similar. It is probably reasonable to test them in a single test/subtest.

[jhenderson]

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

1

I think it would make sense to either have all fields tested in a single file, or all in separate files. I don't think the hex nature of these ones is sufficient reason for these two to be diffferent. If doing them all in a single file, you could do everything using decimal, and then show that hex can be used too in an additional single case at the end, I guess.

3

Perhaps we don't need the header check at all actually? As it is, it looks like it's verifying something important, but the null entry and symbol count should be covered elsewhere, I reckon.

11

Perhaps ---

[grimar]

• Addressed review comments.

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

1

I've splitted this test.

3

I've removed it.

[jhenderson]

Looks good, with one possible set of suggestions.

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

15–16 ↗

(On Diff #311847)

Up to you, but here and below, you could probably reduce the amount of stuff needed by the test by using regex wildcards/numeric expressions to skip much of the check. For example, see the inline edit. The same technique could probably be used for each of the other cases too, in both tests.

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

15–16 ↗

(On Diff #311847)

Equivalent suggestion to above.

[grimar]

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

15–16 ↗

(On Diff #311847)

Thanks for the suggestion! I've applied it.

llvm/include/llvm/ObjectYAML/ELFYAML.h

 };
 
 struct Symbol {
   StringRef Name;
   ELF_STT Type;
   Optional<StringRef> Section;
   Optional<ELF_SHN> Index;
   ELF_STB Binding;
-  llvm::yaml::Hex64 Value;
-  llvm::yaml::Hex64 Size;
+  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

else if (!Sym.Name.empty())
       Symbol.st_name = Strtab.getOffset(ELFYAML::dropUniqueSuffix(Sym.Name));
 
     Symbol.setBindingAndType(Sym.Binding, Sym.Type);
     if (Sym.Section)
       Symbol.st_shndx = toSectionIndex(*Sym.Section, "", Sym.Name);
     else if (Sym.Index)
       Symbol.st_shndx = *Sym.Index;
 
-    Symbol.st_value = Sym.Value;
+    Symbol.st_value = Sym.Value.getValueOr(yaml::Hex64(0));
     Symbol.st_other = Sym.Other ? *Sym.Other : 0;
-    Symbol.st_size = Sym.Size;
+    Symbol.st_size = Sym.Size.getValueOr(yaml::Hex64(0));
   }
 
   return Ret;
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,
                                              SymtabType STType,

llvm/lib/ObjectYAML/ELFYAML.cpp

 
 void MappingTraits<ELFYAML::Symbol>::mapping(IO &IO, ELFYAML::Symbol &Symbol) {
   IO.mapOptional("Name", Symbol.Name, StringRef());
   IO.mapOptional("StName", Symbol.StName);
   IO.mapOptional("Type", Symbol.Type, ELFYAML::ELF_STT(0));
   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("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);

llvm/test/tools/obj2yaml/ELF/symbol.yaml

+## This is a test case to check how tool dumps symbols.
+
+## Check that we only dump Size and Value keys when
+## their values are not 0.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: obj2yaml %t1 | FileCheck %s --check-prefix=FIELDS-DEF
+
+# FIELDS-DEF:      Symbols:
+# FIELDS-DEF-NEXT:   - Name:  foo
+# FIELDS-DEF-NEXT:   - Name:  bar
+# FIELDS-DEF-NEXT:     Value: 0x1
+# FIELDS-DEF-NEXT:     Size:  0x1
+# FIELDS-DEF-NEXT: ...
+
+!ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL
+Symbols:
+  - Name:  foo
+    Size:  0x0
+    Value: 0x0
+  - Name:  bar
+    Size:  0x1
+    Value: 0x1

llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml

+## Check we can set different values and sizes for symbols.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --symbols %t | FileCheck %s
+
+# CHECK:      Symbol table '.symtab' contains 6 entries:
+# CHECK-NEXT: Num:    Value       Size Type    Bind   Vis       Ndx Name
+# CHECK-NEXT: 0: 0000000000000000 0    NOTYPE  LOCAL  DEFAULT   UND
+
+!ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL
+Symbols:
+## No "Size" or "Value" keys were set. Check it is the
+## same as if we would set them to 0.
+  - Name:  aaa
+# CHECK-NEXT: 1: 0000000000000000 0    NOTYPE  LOCAL  DEFAULT   UND aaa
+## Both "Size" and "Value" are explicitly set to 0x0.
+  - Name:  bbb
+    Value: 0x0
+    Size:  0x0
+## Check we can use the "=<none>" syntax.
+# CHECK-NEXT: 2: 0000000000000000 0    NOTYPE  LOCAL  DEFAULT   UND bbb
+  - Name:  ccc
+    Value: [[EVAL=<none>]]
+    Size:  [[ESIZE=<none>]]
+# CHECK-NEXT: 3: 0000000000000000 0    NOTYPE  LOCAL  DEFAULT   UND ccc
+## Both "Size" and "Value" are explicitly set to an arbitraty value.
+## Here we use UINT64_MAX to check this boundary case.
+  - Name:  ddd
+    Value: 0xffffffffffffffff
+    Size:  0xffffffffffffffff
+# CHECK-NEXT: 4: ffffffffffffffff -1   NOTYPE  LOCAL  DEFAULT   UND ddd
+## The same as previous, but decimal values were used.
+  - Name:  eee
+    Value: 18446744073709551615
+    Size:  18446744073709551615
+# CHECK-NEXT: 5: ffffffffffffffff -1   NOTYPE  LOCAL  DEFAULT   UND eee

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->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;
   S.Binding = Sym->getBinding();
 
   Expected<StringRef> SymbolNameOrErr =
       getUniquedSymbolName(Sym, StrTable, SymTab);
   if (!SymbolNameOrErr)
     return SymbolNameOrErr.takeError();
   S.Name = SymbolNameOrErr.get();