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

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D37391