[llvm-objcopy] [COFF] Implmement --strip-unneeded and -x/--discard-all for symbols

[jhenderson]

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

1 ↗

(On Diff #180799)

The ELF side of llvm-objcopy also strips undefined local symbols that are unreferenced with --strip-unneeded. Should that be the same in COFF?

8–9 ↗

(On Diff #180799)

Just to confirm, this is what GNU objcopy does? The ELF side attempts to strip local symbols in this case whether they're referenced or not.

Another difference between the two is that undefined locals are stripped by strip-unneeded, but not discard-all.

11 ↗

(On Diff #180799)

I have a marginal preference to use a different output file name for each case. This allows a user to inspect the corresponding output file, should the need arise, without having to mess about with the test.

[mstorsjo]

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

8–9 ↗

(On Diff #180799)

GNU objcopy doesn't strip referenced local symbols, but undefined locals (which I wasn't even aware of being a thing) does indeed seem to have the difference you mention. I'll implement that.

11 ↗

(On Diff #180799)

Yes, especially as the output actually should be different in this case, after adding a testcase of undefined local symbols.

[mstorsjo]

Did the suggested changes to the test, added the distinction that --discard-all keeps undefined local symbols.

[jhenderson]

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

8–9 ↗

(On Diff #180799)

Okay, as there's now a behaviour difference, please split discard-all into a separate test.

[mstorsjo]

Split testes into two separate files, with one shared yaml source in the Inputs directory.

[jhenderson]

test/tools/llvm-objcopy/COFF/Inputs/discard-locals.yaml

15 ↗

(On Diff #181236)

Thinking about it, it's probably worth having an undefined external too.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

53–54 ↗

(On Diff #181236)

GNU objcopy (probably) doesn't have a "Config.DiscardAll" member. Change this and the StripUnneeded reference to switch names (i.e. --discard-all/--strip-unneeded).

[mstorsjo]

Added an undefined external to the tests, changed the code comment to use CLI parameter names.

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/COFF/Inputs/discard-locals.yaml

+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     E800000000C3C3C3
+    Relocations:     
+      - VirtualAddress:  1
+        SymbolName:      local_referenced
+        Type:            IMAGE_REL_AMD64_REL32
+symbols:         
+  - Name:            external
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            external_undefined
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            local_unreferenced
+    Value:           6
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            local_referenced
+    Value:           7
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            local_undefined_unreferenced
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...

llvm/trunk/test/tools/llvm-objcopy/COFF/discard-all.test

+RUN: yaml2obj %p/Inputs/discard-locals.yaml > %t.in.o
+
+RUN: llvm-objdump -t %t.in.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-PRE
+
+RUN: llvm-objcopy --discard-all %t.in.o %t.out.o
+RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefixes=SYMBOLS
+
+RUN: llvm-objcopy -x %t.in.o %t.out-x.o
+RUN: cmp %t.out.o %t.out-x.o
+
+RUN: cp %t.in.o %t.strip-x.o
+RUN: llvm-strip -x %t.strip-x.o
+RUN: cmp %t.out.o %t.strip-x.o
+
+RUN: cp %t.in.o %t.strip-discard-all.o
+RUN: llvm-strip --discard-all %t.strip-discard-all.o
+RUN: cmp %t.out.o %t.strip-discard-all.o
+
+SYMBOLS: SYMBOL TABLE:
+SYMBOLS-NEXT: external
+SYMBOLS-NEXT: external_undefined
+SYMBOLS-PRE-NEXT: local_unreferenced
+SYMBOLS-NEXT: local_referenced
+SYMBOLS-NEXT: local_undefined_unreferenced
+SYMBOLS-EMPTY:

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

+RUN: yaml2obj %p/Inputs/discard-locals.yaml > %t.in.o
+
+RUN: llvm-objdump -t %t.in.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-PRE
+
+RUN: llvm-objcopy --strip-unneeded %t.in.o %t.out.o
+RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefix=SYMBOLS
+
+SYMBOLS: SYMBOL TABLE:
+SYMBOLS-NEXT: external
+SYMBOLS-NEXT: external_undefined
+SYMBOLS-PRE-NEXT: local_unreferenced
+SYMBOLS-NEXT: local_referenced
+SYMBOLS-PRE-NEXT: local_undefined_unreferenced
+SYMBOLS-EMPTY:

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

 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   // If we need to do per-symbol removals, initialize the Referenced field.
-  if (!Config.SymbolsToRemove.empty())
+  if (Config.StripUnneeded || Config.DiscardAll ||
+      !Config.SymbolsToRemove.empty())
     if (Error E = Obj.markSymbols())
       return E;
 
   // Actually do removals of symbols.
   Obj.removeSymbols([&](const Symbol &Sym) {
     if (is_contained(Config.SymbolsToRemove, Sym.Name)) {
       // Explicitly removing a referenced symbol is an error.
       if (Sym.Referenced)
         reportError(Config.OutputFilename,
                     make_error<StringError>(
                         "not stripping symbol '" + Sym.Name +
                             "' because it is named in a relocation.",
                         llvm::errc::invalid_argument));
       return true;
     }
 
+    if (!Sym.Referenced && Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC) {
+      if (Config.StripUnneeded)
+        return true;
+      // GNU objcopy keeps referenced local symbols and external symbols
+      // if --discard-all is set, similar to what --strip-unneeded does,
+      // but undefined local symbols are kept when --discard-all is set.
+      if (Config.DiscardAll && Sym.Sym.SectionNumber != 0)
+        return true;
+    }
+
     return false;
   });
   return Error::success();
 }
 
 void executeObjcopyOnBinary(const CopyConfig &Config,
                             object::COFFObjectFile &In, Buffer &Out) {
   COFFReader Reader(In);