[PATCH] D49206: [llvm-objcopy] Add support for large indexes

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with the nits, assuming the tests aren't realistic without spending hours at it.



================
Comment at: llvm/test/tools/llvm-objcopy/remove-shndx.test:1
+RUN: python %p/Inputs/ungzip.py %p/Inputs/many-sections.o.gz > %t
+RUN: llvm-objcopy -R .symtab_shndx %t %t2
----------------
Probably worth putting a comment at the start of this test, to say what it is doing, because at the moment the test name is "remove-shndx", but the test doesn't actually do so!


================
Comment at: llvm/tools/llvm-objcopy/Object.cpp:203
+// Large indexes force us to clarify exactly what this function should do. This
+// function should return the proper value of st_shndx.
 uint16_t Symbol::getShndx() const {
----------------
jhenderson wrote:
> I'm not sure "proper value of st_shndx" is quite clear enough. Maybe "the value that will appear in st_shndx" might be better?
Nit: will appear st_shndx -> will appear in st_shndx


================
Comment at: llvm/tools/llvm-objcopy/Object.cpp:726
+          Index,
+          "Symbol '" + Name + "' is defined in invalid section with index " +
+              Twine(Index));
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Hmm... not sure about this error message, since the section isn't invalid, rather the section //index// is. So, maybe something like "Symbol 'bar' has an invalid section index 12345".
> > 
> > I assume that testing this would be unrealistic due to the size of the required test input?
> This is actually testable quite easily by uploading a specially constructed binary (which I'm getting pretty good at these days). Basically construct a binary that uses SHN_XINDEX and then puts an overly large index in the shndx table despite there being only 1 symbol. This would require just a bit of yaml2obj and some hex editing. The problem is that constructing such a binary is liable to take me 2-3 hours and I don't want to spend that time just to test one error message. 
Okay, fair enough. I won't force it. I wonder if maybe this is an indication we need a "relaxed" mode for yaml2obj that allows us to write arbitrary values to the file? Anyway, not really part of this, more just idle musing again.

I think my ideal scenario would be able to write a symbol with a custom section index to the symbol table, even if that doesn't refer to a real section index.


================
Comment at: llvm/tools/llvm-objcopy/Object.cpp:712-713
+      if (SymTab->getShndxTable() == nullptr)
+        error("Symbol '" + Name +
+              "' has index SHN_XINDEX but no SHT_SYMTAB_SHNDX section exists.");
+      if (ShndxData.data() == nullptr) {
----------------
Do we have a sensible way to test this, without binary editing something?


================
Comment at: llvm/tools/llvm-objcopy/Object.cpp:720-721
+        if (ShndxData.size() != Symbols.size())
+          error("Symbol section index table does not have the same number of "
+                "entries as the symbol table.");
+      }
----------------
Ditto. Is a test here feasible?


Repository:
  rL LLVM

https://reviews.llvm.org/D49206