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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 7 inline comments as done.
mstorsjo added inline comments.
Herald added a reviewer: rupprecht.


================
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
+
----------------
jhenderson wrote:
> These two could be folded together into one test.
Ok, will do.


================
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
----------------
jhenderson wrote:
> 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.
As --strip-all-gnu vs --strip-all is for ELF specific details in how/what to strip, I don't think I'll need to make any difference between them, so I can just compare the output objects instead. I'll add tests for llvm-strip and -S as well.


================
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
----------------
jhenderson wrote:
> What's the purpose of this symbol?
Just to provide a bit more test data, to show that both defined and undefined symbols are stripped.


================
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;
   }
----------------
jhenderson wrote:
> 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?
Done in D56584.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56481