[yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE

[jhenderson]

Summary

In order to test tool handling of invalid section indexes, I need to create an object contianing such an invalid section index. I could create a hex-edited binary, but having the ability to use yaml2obj is preferable. Prior to this change, yaml2obj would reject any explicit section indexes less than SHN_LORESERVE. This patch changes it to allow any value.

I had to change the test to use llvm-readelf instead of llvm-readobj, because llvm-readobj does not like invalid section indexes. I've also expanded the test to show that the most common SHN_* values are accepted (SHN_UNDEF, SHN_ABS, SHN_COMMON).

[grimar]

I thought before about some kind of conflict we might see in situations like this.
From one side we might want to keep (and maybe improve) the validating because it is probably a useful thing in general.
But we still want to have a way to produce invalid outputs when we want, as this patch does.

Should we instead of removing the validation code add an option to yaml2obj, something like -no-validate?

[jhenderson]

In D58445#1404051, @grimar wrote:

I thought before about some kind of conflict we might see in situations like this.
From one side we might want to keep (and maybe improve) the validating because it is probably a useful thing in general.
But we still want to have a way to produce invalid outputs when we want, as this patch does.

Should we instead of removing the validation code add an option to yaml2obj, something like -no-validate?

Hmm... interesting thought. In this case, I think if you've explicitly used Index, you probably know what you are doing by specifying an explicit section index outside the reserved range, instead of using one of the SHN_* values or using the Section field. I could be persuaded otherwise though. On a related note, without this change, you cannot specify Index: SHN_UNDEF, because SHN_UNDEF is less than SHN_LORESERVE. It's not strictly necessary, because the default is undefined, but it's still slightly weird behaviour.

[jhenderson]

By the way, I could see a use for a validate/no-validate option in other cases. Perhaps with the Index field it would validate that the index is either a valid section index or a reserved value.

[grimar]

@jhenderson wrote:
In this case, I think if you've explicitly used Index, you probably know what you are doing by specifying an explicit section index outside the reserved range, instead of using one of the SHN_* values or using the Section field.

Yes, for this case you are right I think.

LGTM.

[jhenderson]

Thanks @grimar! I'm going to sit on this for a day or two and see if any other reviewers want to chime in before I go ahead.

[jakehehrlich]

This emphatically looks good to me. Thanks for doing this.

llvm/trunk/lib/ObjectYAML/ELFYAML.cpp

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

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

 # RUN: yaml2obj %s > %t
-# RUN: llvm-readobj -symbols %t | FileCheck %s
+# RUN: llvm-readelf -symbols %t | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
 Symbols:
   Global:
-    - Name:     test
+    - Name:     absolute1
       Index:    SHN_ABS
       Value:    0x1234
+    - Name:     absolute2
+      Index:    0xfff1
+      Value:    0x4321
+    - Name:     common1
+      Index:    SHN_COMMON
+    - Name:     common2
+      Index:    0xfff2
+    - Name:     good
+      Index:    0x1
+    - Name:     bad
+      Index:    0x42
+    - Name:     undef1
+      Index:    SHN_UNDEF
+    - Name:     undef2
+      Index:    0
 
-#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:]
+# 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