[PATCH] D46896: [llvm-objcopy] Add --strip-unneeded option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Since the ELF type is no longer relevant, could you remove all but one of the tests, please.



================
Comment at: test/tools/llvm-objcopy/strip-unneeded-rel.test:44
+      Value:    0x1010
+  Global:
+    - Name:     foobar
----------------
Please add an undefined weak and a defined global to this test. Also a STT_FILE symbol and STT_SECTION symbol to show they are not stripped.


================
Comment at: tools/llvm-objcopy/Object.cpp:264-265
+Symbol *SymbolTableSection::getSymbolByIndex(uint32_t Index) {
+  if (Symbols.size() <= Index)
+    error("Invalid symbol index: " + Twine(Index));
+  return Symbols[Index].get();
----------------
I might be inclined to make this check into a separate shared method, to avoid risk of the two diverging.


================
Comment at: tools/llvm-objcopy/Object.cpp:662
+    Symbol *Sym = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
+    Sym->RelocRefs += 1;
+    ToAdd.RelocSymbol = Sym;
----------------
`++Sym->RelocRefs` instead of `+= 1`?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:392
 
+      // TODO: We might handle the 'zero symbol' in a different way
+      // by probably handling it the same way as we handle 'zero section' ?
----------------
I'd refer to the "zero symbol" as the "null symbol". Ditto the section.


Repository:
  rL LLVM

https://reviews.llvm.org/D46896