[yaml2obj][ELF] Add support for symbol indexes greater than SHN_LORESERVE

[jakehehrlich]

Summary

Right now Symbols must be either undefined or defined in a specific section. Some symbols have section indexes like SHN_ABS however. This change adds support for outputting symbols that have such section indexes.

[jakehehrlich]

Uploaded slightly wrong diff. Fixed.

[mcgrathr]

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

SHN_XINDEX is a special animal unlike other special cases. It means the real shndx value is elsewhere. See SHN_XINDEX handling elsewhere in the code base (or the ELF spec).

SHN_UNDEF, SHN_ABS, and SHN_COMMON are standard special cases. Those just go through as they are.
(Note that SHN_UNDEF is not in the [SHN_LORESERVE,SHN_HIRESERVE] range like all the others here are.)

SHN_{LO,HI}* are not actual values, but markers for the ranges used for machine-specific and OS-specific values.
It doesn't make sense to match them.

I think you need to handle each such value specially, because the correct way to treat it will depend on the specific machine/OS-specific definition.
All the ones I've seen are basically variants of SHN_COMMON and are handled the same way (just pass it through). But I think the only safe thing is to go case by case for handling specific ones we understand, and error out if you encounter one we don't know how to handle.

[jakehehrlich]

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

This is just code that translates strings of t he form " SHN_* " to the appropriate value for use anywhere in yaml2elf. It doesn't handle what happens in these cases any further than that. I think there's an argument that SHN_{LO,HI}* shouldn't be here since you would never want to use those names when building an ELF object. The rest of the file however includes such values however (see SHT_LOOS and SHT_LOPROC in the equivliant code for section types) so I followed suit.

In general ELF_SHN should be the type of values with names SHN_* in this code base; even the ones which are not logically values you would want to use in a yaml file to produce an ELF.

[mcgrathr]

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

Understood. I think you still need to handle SHN_XINDEX specially though, because it has a structural meaning.
It probably makes more sense for yaml files to list large literal section numbers and have yaml2obj do the SHN_XINDEX encoding for them automagically. But maybe yaml2elf is meant to test that encoding logic too, in which case you'd have to spell out SHN_XINDEX in symtab entries and then spell out the SHT_SYMTAB_SHNDX section with its contents explicitly.

You should certainly handle all the reserved-range SHN_* names in include/llvm/BinaryFormat/ELF.h (there are more that binutils groks).

[jakehehrlich]

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

I think sean would likely say that yaml2obj should handle large indexes automagically, following the logic that it should be hard to produce invalid files. That being the case I don't think I want to support large indexes in this patch. Perhaps I should throw an error if the user creates a large SHN_XINDEX? As is you can't produce a valid file that uses large section indexes because there's no support for SHT_SYMTAB_SHNDX (I would have to add that).

[mcgrathr]

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

It sounds fine to have this change only add support for the known SHN_* names. Perhaps it should recognize SHN_XINDEX as a name, but specifically refuse to write it out, since you should never use it explicitly if yaml2obj should not easily produce invalid ELF files. You should also make sure that it refuses numeric section numbers >= SHN_LORESERVE. A separate change to produce SHT_SYMTAB_SHNDX automagically for large number section numbers makes sense.

[jakehehrlich]

1. Added support for the rest of the SHN_* values in ELF.h
2. An error should now be thrown if the user tries to use SHN_XINDEX

[jakehehrlich]

1. Added check to make sure the users uses "Section: <section name>" to specify section indexes and not "Index: <value>"

[jakehehrlich]

Needed to check that Symbol.Index was set first before comparing it.

[mcgrathr]

Not being expert in the yaml2obj code base, this looks good to me with the caveats below.

include/llvm/ObjectYAML/ELFYAML.h

53 ↗

(On Diff #113610)

This is actually a 16-bit field in ElfNN_Sym.

lib/ObjectYAML/ELFYAML.cpp

743 ↗

(On Diff #113610)

It looks like llvm::Optional has both operator* and operator==/operator< et al that essentially forward the operator to the value that operator* would yield (in the has-value case, anyway--the overall semantics of the comparison operators on Optional are a bit weird, and I think there's some implicit copy construction in there too), so either Symbol.Index or *Symbol.Index works here, but you should be consistent, and *Symbol.Index looks more natural next to the Symbol.Index->bool test (and I suspect is the only pattern with sensible semantics when dealing with Optional<T> where T is not a trivial POD type like this one).

[jakehehrlich]

I agree that it should be *Symbol.Index. This was a mistake on my part.

1. Fixed not dereferencing the optional value
2. Changes indexes to be 16-bit instead of 32-bit

Ideally I'd like to have Sean or Simon look at this first before landing.

[jakehehrlich]

ping

[silvas]

Sorry for the delay.

One suggestion for paring down the recognized SHN_* list and how I'm personally conceptualizing my thought process for what to include there.

This LGTM.

lib/ObjectYAML/ELFYAML.cpp

491 ↗

(On Diff #113576)

I agree with jake that the SHN_XINDEX is purely an artifact of the file encoding, so we probably don't want to allow the user to explicitly provide it (seems too error-prone)
It seems like the only purpose for supporting SHN_XINDEX explicitly specified in the YAML file would be to test our SHN_XINDEX handling, which seems like it is better done with a different sort of test (probably just checked in binaries since it's a very narrow and specific thing to the binary format; literally just a historical "oops, we should have made this field have more bits" workaround). Since yaml2obj is intended as a testing tool, its inputs should be pretty small and so we shouldn't be needing SHN_XINDEX.

It might be interesting to add an error check/diagnostic in the case of too many sections in the input yaml file, but for now just omitting SHN_XINDEX from the list of possibilities seems fine.

There probably isn't much harm in keeping in the SHN_{LO,HI}* values around, though I assume the common usage of them would be something like SHN_LORESERVE + <N> or similar so a hex value would likely still be needed (or adding support for a semantic constant like has been done for the hexagon ones). So I'd probably lean towards removing the SHN_{LO,HI}* values since it would make the test case difficult to read (for example, of somebody used SHN_LORESERVE instead of SHN_HEXAGON_SCOMMON which has the same numerical value). On the other hand, that breaks the simplicity of "what should we list in ScalarEnumerationTraits<ELFYAML::ELF_SHN>::enumeration", which is currently just every SHN_* it seems. So if we omit some we should definitely have a comment explaining the rationale for what values we allow/omit.

For now, let's just follow a YAGNI principle and just support SHN_ABS since it seems that that's all you really need (and a comment to that effect in ScalarEnumerationTraits<ELFYAML::ELF_SHN>::enumeration). SHN_COMMON is a likely eventual need too. But SHN_UNDEF probably isn't since we handle undef symbols already by omitting the section (so in a sense it is a default value, though conceivably we could allow it to be explicitly specified).

llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h

 // Just use 64, since it can hold 32-bit values too.
 LLVM_YAML_STRONG_TYPEDEF(uint64_t, ELF_EF)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, ELF_PF)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, ELF_SHT)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, ELF_REL)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_RSS)
 // Just use 64, since it can hold 32-bit values too.
 LLVM_YAML_STRONG_TYPEDEF(uint64_t, ELF_SHF)
+LLVM_YAML_STRONG_TYPEDEF(uint16_t, ELF_SHN)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STT)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STV)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STO)
 
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, MIPS_AFL_REG)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, MIPS_ABI_FP)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_AFL_EXT)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_AFL_ASE)
struct ProgramHeader {
   llvm::yaml::Hex64 PAddr;
   std::vector<SectionName> Sections;
 };
 
 struct Symbol {
   StringRef Name;
   ELF_STT Type;
   StringRef Section;
+  Optional<ELF_SHN> Index;
   llvm::yaml::Hex64 Value;
   llvm::yaml::Hex64 Size;
   uint8_t Other;
 };
 
 struct LocalGlobalWeakSymbols {
   std::vector<Symbol> Local;
   std::vector<Symbol> Global;
struct ScalarEnumerationTraits<ELFYAML::ELF_SHT> {
   static void enumeration(IO &IO, ELFYAML::ELF_SHT &Value);
 };
 
 template <>
 struct ScalarBitSetTraits<ELFYAML::ELF_SHF> {
   static void bitset(IO &IO, ELFYAML::ELF_SHF &Value);
 };
 
+template <> struct ScalarEnumerationTraits<ELFYAML::ELF_SHN> {
+  static void enumeration(IO &IO, ELFYAML::ELF_SHN &Value);
+};
+
 template <>
 struct ScalarEnumerationTraits<ELFYAML::ELF_STT> {
   static void enumeration(IO &IO, ELFYAML::ELF_STT &Value);
 };
 
 template <>
 struct ScalarEnumerationTraits<ELFYAML::ELF_STV> {
   static void enumeration(IO &IO, ELFYAML::ELF_STV &Value);

 
 template <> struct MappingTraits<ELFYAML::ProgramHeader> {
   static void mapping(IO &IO, ELFYAML::ProgramHeader &FileHdr);
 };
 
 template <>
 struct MappingTraits<ELFYAML::Symbol> {
   static void mapping(IO &IO, ELFYAML::Symbol &Symbol);
+  static StringRef validate(IO &IO, ELFYAML::Symbol &Symbol);
 };
 
 template <>
 struct MappingTraits<ELFYAML::LocalGlobalWeakSymbols> {
   static void mapping(IO &IO, ELFYAML::LocalGlobalWeakSymbols &Symbols);
 };
 
 template <> struct MappingTraits<ELFYAML::Relocation> {

llvm/trunk/lib/ObjectYAML/ELFYAML.cpp

case ELF::EM_X86_64:
     break;
   default:
     // Nothing to do.
     break;
   }
 #undef BCase
 }
 
+void ScalarEnumerationTraits<ELFYAML::ELF_SHN>::enumeration(
+    IO &IO, ELFYAML::ELF_SHN &Value) {
+#define ECase(X) IO.enumCase(Value, #X, ELF::X)
+  ECase(SHN_UNDEF);
+  ECase(SHN_LORESERVE);
+  ECase(SHN_LOPROC);
+  ECase(SHN_HIPROC);
+  ECase(SHN_LOOS);
+  ECase(SHN_HIOS);
+  ECase(SHN_ABS);
+  ECase(SHN_COMMON);
+  ECase(SHN_XINDEX);
+  ECase(SHN_HIRESERVE);
+  ECase(SHN_HEXAGON_SCOMMON);
+  ECase(SHN_HEXAGON_SCOMMON_1);
+  ECase(SHN_HEXAGON_SCOMMON_2);
+  ECase(SHN_HEXAGON_SCOMMON_4);
+  ECase(SHN_HEXAGON_SCOMMON_8);
+#undef ECase
+  IO.enumFallback<Hex32>(Value);
+}
+
 void ScalarEnumerationTraits<ELFYAML::ELF_STT>::enumeration(
     IO &IO, ELFYAML::ELF_STT &Value) {
 #define ECase(X) IO.enumCase(Value, #X, ELF::X)
   ECase(STT_NOTYPE);
   ECase(STT_OBJECT);
   ECase(STT_FUNC);
   ECase(STT_SECTION);
   ECase(STT_FILE);

 };
 
 } // end anonymous namespace
 
 void MappingTraits<ELFYAML::Symbol>::mapping(IO &IO, ELFYAML::Symbol &Symbol) {
   IO.mapOptional("Name", Symbol.Name, StringRef());
   IO.mapOptional("Type", Symbol.Type, ELFYAML::ELF_STT(0));
   IO.mapOptional("Section", Symbol.Section, StringRef());
+  IO.mapOptional("Index", Symbol.Index);
   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()) {
+    return "Index and Section cannot both be specified for Symbol";
+  }
+  if (Symbol.Index && *Symbol.Index == ELFYAML::ELF_SHN(ELF::SHN_XINDEX)) {
+    return "Large indexes are not supported";
+  }
+  if (Symbol.Index && *Symbol.Index < ELFYAML::ELF_SHN(ELF::SHN_LORESERVE)) {
+    return "Use a section name to define which section a symbol is defined in";
+  }
+  return StringRef();
+}
+
 void MappingTraits<ELFYAML::LocalGlobalWeakSymbols>::mapping(
     IO &IO, ELFYAML::LocalGlobalWeakSymbols &Symbols) {
   IO.mapOptional("Local", Symbols.Local);
   IO.mapOptional("Global", Symbols.Global);
   IO.mapOptional("Weak", Symbols.Weak);
 }
 
 static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {

llvm/trunk/test/tools/yaml2obj/symbol-index-invalid.yaml

+# This test insures that the user cannot have both an Index and a Section field.
+# RUN: not yaml2obj %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Content:         "0000000000000000"
+Symbols:
+  Global:
+    - Name:     _start
+      Section:  .text
+      Index:    SHN_ABS

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

+# RUN: yaml2obj %s > %t
+# RUN: llvm-readobj -symbols %t | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Symbols:
+  Global:
+    - Name:     test
+      Index:    SHN_ABS
+      Value:    0x1234
+
+#CHECK:     Symbols [
+#CHECK-NEXT:   Symbol {
+#CHECK-NEXT:    Name:  (0)
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local (0x0)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined (0x0)
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: test (1)
+#CHECK-NEXT:    Value: 0x1234
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Global (0x1)
+#CHECK-NEXT:    Type: None (0x0)
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Absolute (0xFFF1)
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

for (const auto &Sym : Symbols) {
     if (!Sym.Section.empty()) {
       unsigned Index;
       if (SN2I.lookup(Sym.Section, Index)) {
         errs() << "error: Unknown section referenced: '" << Sym.Section
                << "' by YAML symbol " << Sym.Name << ".\n";
         exit(1);
       }
       Symbol.st_shndx = Index;
-    } // else Symbol.st_shndex == SHN_UNDEF (== 0), since it was zero'd earlier.
+    } 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;
     Symbol.st_size = Sym.Size;
     Syms.push_back(Symbol);
   }
 }
 
 template <class ELFT>