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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:491
+  ECase(SHN_COMMON);
+  ECase(SHN_XINDEX);
+  ECase(SHN_HIRESERVE);
----------------
mcgrathr wrote:
> jakehehrlich wrote:
> > mcgrathr wrote:
> > > 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.
> > 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.
> 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).
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D37391