[PATCH] D61672: [llvm-objcopy] Allow strip symtab in executables and DSOs

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Does GNU remove all the symbols or remove the symbol table entirely? The two are slightly different. If the latter, we should look at whether it's specific to the --strip-unneeded switch or more generally when all symbols end up getting discarded.



================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:1
+# RUN: yaml2obj %s > %t
+# RUN: cp %t %t1
----------------
Perhaps worth renaming this test to something like strip-unneeded-all-symbols.test. This test could also do with a brief comment at the top stating its purpose.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:9
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
----------------
Nit: reduce the unnecessary padding amount, here and below.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:20
+    Size:            64
+DynamicSymbols:
+  - Name:     foo
----------------
What's the point of having DynamicSymbols here?


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:29-32
+    Type:     STT_FUNC
+    Size:     8
+    Section:  .text
+    Value:    0x1008
----------------
I think you can simplify this test by removing most of these fields. The only relevant fields are the Binding, Section and Name fields, I believe. Also, what happens for undefined STB_GLOBALs in GNU objcopy? There should be a test for that case here.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:408
          is_contained(Config.UnneededSymbolsToRemove, Sym.Name)) &&
-        isUnneededSymbol(Sym))
+        (Obj.Type == ET_EXEC || Obj.Type == ET_DYN || isUnneededSymbol(Sym)))
       return true;
----------------
What does GNU objcopy do for other ET_* types, e.g. ET_CORE or ET_* values in the OS and PROC-specific ranges? Could this be inverted to != ET_REL?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61672/new/

https://reviews.llvm.org/D61672