[PATCH] D37393: [llvm-objcopy] Add support for special section indexes in symbol table greater than SHN_LORESERVE

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

I'm not closely familiar with this code base, so it should probably still get some review from someone who has reviewed the objcopy implementation so far.
But just looking at the apparent logic in this change, it looks OK to me with the caveats noted below.



================
Comment at: tools/llvm-objcopy/Object.cpp:200
 
+bool isSupportedSectionIndex(uint32_t Index) {
+  switch(Index) {
----------------
This seems an oddly general-sounding name for this.
Perhaps isValidReservedSectionIndex?


================
Comment at: tools/llvm-objcopy/Object.cpp:239
+        error("Symbol '" + Name +
+              "' has unsupported value greater than SHN_LORESERVE: " +
+              Twine(Sym.st_shndx));
----------------
It's >=, not >.


================
Comment at: tools/llvm-objcopy/Object.h:119
   SectionBase *DefinedIn;
+  uint32_t SectionIndex;
   uint32_t Index;
----------------
Since this is currently not actually the section index per se, I'd prefer not to call it that.
It's the st_shndx value, but it's not an index if it's a reserved value.
In fact, arguably it should be ElfNN_Section type, i.e. uint16_t.
And have a comment about the meaning.
When you actually support 32-bit section indices, you can have a field called SectionIndex that's 32 bits.
(You'll still need to separately represent when st_shndx is a reserved value, so as to distinguish it from when st_shndx is SHN_XINDEX and the 32-bit SHT_SYMTAB_SHNDX entry happens to be in the [SHN_LORESERVE,SHN_HIRESERVE] range.)


Repository:
  rL LLVM

https://reviews.llvm.org/D37393