[PATCH] D56840: [llvm-objcopy] [COFF] Implement --only-keep-debug

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/only-keep-debug.yaml:4
+# RUN: llvm-objcopy --only-keep-debug %t.in.exe %t.out.exe
+# RUN: llvm-readobj -sections %t.out.exe | FileCheck %s --check-prefix=SECTIONS
+# RUN: llvm-objdump -t %t.out.exe | FileCheck %s --check-prefix=SYMBOLS
----------------
-sections -> --sections


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:34-36
+static bool isNondebugKeepableSection(const Section &Sec) {
+  return Sec.Name == ".buildid";
+}
----------------
I think you should just inline this.


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:39
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+  if (Config.OnlyKeepDebug) {
+    // For --only-keep-debug, we keep all other sections, but remove
----------------
Would it make more sense for this to be after section removal, so that it has less work to do?


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:41
+    // For --only-keep-debug, we keep all other sections, but remove
+    // their content data. The VirtualSize field in the section header
+    // is kept intact.
----------------
Remove the word "data" here, unless "content data" is a COFF phrasing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56840