[llvm-objcopy] [COFF] Implement --strip-debug

[mstorsjo]

Summary

Also remove sections similarly for --strip-all, --discard-all, --strip-unneeded.

[jhenderson]

test/tools/llvm-objcopy/COFF/strip-debug.test

1 ↗

(On Diff #182241)

Are you planning on using this yaml file for other tests? Otherwise, it seems confusingly named.

3 ↗

(On Diff #182241)

Nit, you're being inconsistent with using single and double-dashes for your options in this file. I prefer double, to avoid theoretical issues with grouping of single-letter options.

7 ↗

(On Diff #182241)

What is SECTIONS-POST for, here and below? I can't see any check patterns for it.

[mstorsjo]

test/tools/llvm-objcopy/COFF/strip-debug.test

1 ↗

(On Diff #182241)

I meant to use it for multiple tests, but it was enough with one, as all these switches turned out to do the same for sections. Will fix.

[mstorsjo]

Fixed @jhenderson's suggestions.

[jhenderson]

LGTM, assuming that this is the same behaviour as GNU objcopy.

[rupprecht]

test/tools/llvm-objcopy/COFF/strip-debug.yaml

1 ↗

(On Diff #182362)

Does this test run? I thought it had to end in ".test" to run.
https://github.com/llvm/llvm-project/blob/master/llvm/test/lit.cfg.py#L25

[mstorsjo]

test/tools/llvm-objcopy/COFF/strip-debug.yaml

1 ↗

(On Diff #182362)

Oh crap, you're right. I'm used to working on tests in e.g. lld, where .yaml already is added to that list of suffixes, and where the convention is that if the file actually contains yaml input data, it's named .yaml, otherwise .test.

I've already committed one test with a .yaml suffix though. Do you want me to add a lit.local.cfg that adds yaml to the list (kind of like https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/lit.local.cfg), or should I rename the existing test to .test? (I didn't notice since I've been running the tests with llvm-lit path/to/llvm-objcopy/COFF/*.)

[mstorsjo]

test/tools/llvm-objcopy/COFF/strip-debug.yaml

1 ↗

(On Diff #182362)

I presume renaming to .test is easiest, and matches the ELF backend's tests, so I'll rename the existing test in one NFC commit and rename the newly added tests before I commit them as well.

[rupprecht]

Accepted assuming the test case passes when renamed to .test :)

test/tools/llvm-objcopy/COFF/strip-debug.yaml

1 ↗

(On Diff #182362)

SGTM -- I think just renaming these to .test is better.

I wouldn't strongly object to allowing .yaml as a test case extension if others are used to it, but it seems a little weird to me...

llvm/trunk/test/tools/llvm-objcopy/COFF/strip-debug.test

+# RUN: yaml2obj %s > %t.in.o
+#
+# RUN: llvm-objdump --section-headers %t.in.o | FileCheck %s --check-prefixes=SECTIONS,SECTIONS-PRE
+# RUN: llvm-objdump -t %t.in.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-PRE
+#
+# RUN: llvm-objcopy --strip-debug %t.in.o %t.out.o
+# RUN: llvm-objdump --section-headers %t.out.o | FileCheck %s --check-prefixes=SECTIONS
+# RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefixes=SYMBOLS
+#
+# Test that --strip-all, --strip-all-gnu, --discard-all and --strip-unneeded,
+# plus llvm-strip without arguments all produce a similiar set of sections
+# (while they remove symbols differently).
+#
+# RUN: llvm-objcopy --strip-all %t.in.o %t.strip-all.o
+# RUN: llvm-objdump --section-headers %t.strip-all.o | FileCheck %s --check-prefixes=SECTIONS
+#
+# RUN: llvm-objcopy --strip-all-gnu %t.in.o %t.strip-all-gnu.o
+# RUN: llvm-objdump --section-headers %t.strip-all-gnu.o | FileCheck %s --check-prefixes=SECTIONS
+#
+# RUN: llvm-objcopy --discard-all %t.in.o %t.discard-all.o
+# RUN: llvm-objdump --section-headers %t.discard-all.o | FileCheck %s --check-prefixes=SECTIONS
+#
+# RUN: llvm-objcopy --discard-all %t.in.o %t.strip-unneeded.o
+# RUN: llvm-objdump --section-headers %t.strip-unneeded.o | FileCheck %s --check-prefixes=SECTIONS
+#
+# SECTIONS:        Sections:
+# SECTIONS-NEXT:   Idx Name
+# SECTIONS-NEXT:     0      .text
+# SECTIONS-NEXT:     1      .data
+# SECTIONS-NEXT:     2      .bss
+# SECTIONS-NEXT:     3      .xdata
+# SECTIONS-NEXT:     4      .reloc
+# SECTIONS-PRE-NEXT: 5      .debug_discardable
+# SECTIONS-NEXT:     {{.*}} .debug_undiscardable
+# SECTIONS-NEXT:     {{.*}} .llvm_addrsig
+# SECTIONS-EMPTY:
+#
+# Test that --strip-debug doesn't remove e.g. unreferenced local symbols.
+#
+# SYMBOLS:          SYMBOL TABLE:
+# SYMBOLS-NEXT:     external
+# SYMBOLS-NEXT:     local_unreferenced
+# SYMBOLS-PRE-NEXT: debug_discardable_sym
+# SYMBOLS-NEXT:     debug_undiscardable_sym
+# SYMBOLS-EMPTY:
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .data
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .bss
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .xdata
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .reloc
+    Characteristics: [ IMAGE_SCN_MEM_DISCARDABLE ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .debug_discardable
+    Characteristics: [ IMAGE_SCN_MEM_DISCARDABLE ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .debug_undiscardable
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     00000000
+  - Name:            .llvm_addrsig
+    Characteristics: [ IMAGE_SCN_LNK_REMOVE ]
+    Alignment:       4
+    SectionData:     00000000
+symbols:         
+  - Name:            external
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            local_unreferenced
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            debug_discardable_sym
+    Value:           0
+    SectionNumber:   6
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            debug_undiscardable_sym
+    Value:           0
+    SectionNumber:   7
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

llvm/trunk/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
+static bool isDebugSection(const Section &Sec) {
+  return Sec.Name.startswith(".debug");
+}
+
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
+    if (Config.StripDebug || Config.StripAll || Config.StripAllGNU ||
+        Config.DiscardAll || Config.StripUnneeded) {
+      if (isDebugSection(Sec) &&
+          (Sec.Header.Characteristics & IMAGE_SCN_MEM_DISCARDABLE) != 0)
+        return true;
+    }
+
     if (is_contained(Config.ToRemove, Sec.Name))
       return true;
 
     return false;
   });
 
   // StripAll removes all symbols and thus also removes all relocations.
   if (Config.StripAll || Config.StripAllGNU)