[PATCH] D56481: [llvm-objcopy] [COFF] Implement --strip-all[-gnu] for symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/strip-all.yaml:7-8
+# RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefix=SYMBOLS
+# RUN: llvm-readobj -relocs %t.out.o | FileCheck %s --check-prefix=RELOCS
+# RUN: llvm-readobj -file-headers %t.out.o | FileCheck %s --check-prefix=CHARS
+
----------------
These two could be folded together into one test.


================
Comment at: test/tools/llvm-objcopy/COFF/strip-all.yaml:10
+
+# RUN: llvm-objcopy --strip-all-gnu %t.in.o %t.out.o
+# RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefix=SYMBOLS
----------------
Do you anticipate --strip-all-gnu and and --strip-all doing the same thing? If so, it's probably clearer and easier to run cmp on the output of the two objcopy commands. Also, you don't appear to have any check for the short-form of the option (-S) or llvm-strip.


================
Comment at: test/tools/llvm-objcopy/COFF/strip-all.yaml:41-46
+  - Name:            external
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
----------------
What's the purpose of this symbol?


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:162
     PointerToSymbolTable = 0;
-    // For executables, skip the length field of an empty string table.
-    if (Obj.IsPE)
-      StrTabSize = 0;
+    StrTabSize = 0;
   }
----------------
mstorsjo wrote:
> The existing code here turned out to be subtly broken when writing an object file with no symbols, so I have to update this part of COFFWriter as part of this change.
If it's already broken, should this be a separate and previous change?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56481