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

[mstorsjo]

Summary

This operation removes all relocations, which is disastrous for object files, but that's probably expected.

[mstorsjo]

tools/llvm-objcopy/COFF/Writer.cpp

164

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.

[jhenderson]

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

8–9

These two could be folded together into one test.

11

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.

42–47

What's the purpose of this symbol?

tools/llvm-objcopy/COFF/Writer.cpp

164

If it's already broken, should this be a separate and previous change?

[mstorsjo]

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

8–9

Ok, will do.

11

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.

42–47

Just to provide a bit more test data, to show that both defined and undefined symbols are stripped.

tools/llvm-objcopy/COFF/Writer.cpp

164

Done in D56584.

[mstorsjo]

Split out the general bugfix, did the suggested changes to the test.

[jhenderson]

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

9

--strip-out-gnu?

[mstorsjo]

Fixed the --strip-all-gnu typo in the test comment.

[jhenderson]

Okay, this LGTM, although I don't know enough about the characteristics stuff to be able to comment on that, so please get somebody else to review that bit.

[rupprecht]

When doing this, GNU objcopy sets a few Characteristics flags as well - I'm only setting one of them as the other ones are documented to be deprecated. I'd like to have @rnk's opinion on this part.

My suggestion would be to continue setting the deprecated characteristics flags for --strip-all-gnu, but not for --strip-all, if I understand the meaning of the difference between the two flags. --strip-all-gnu should be for more extreme compatibility between gnu, --strip-all is allowed to be a bit more aggressive.
But I have never heard of characteristics before, maybe this is a case where we want to break GNU objcopy compatibility (we don't aim to be bit-for-bit compatible; even GNU-matching flags will different in minor aspects).
Probably best to let @rnk comment on characteristics.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

41

It looks like this is not always supposed to be added; have you seen https://sourceware.org/bugzilla/show_bug.cgi?id=11396?

43

What about IMAGE_FILE_DEBUG_STRIPPED? Do you know if GNU objcopy sets that in any cases?

tools/llvm-objcopy/COFF/Writer.cpp

55

IMO this if/else block is actually easier to read with a ternary:

S.Header.PointerToRelocations = (S.Header.NumberOfRelocations > 0) ? FileSize : 0;

[mstorsjo]

In D56481#1354845, @rupprecht wrote:

When doing this, GNU objcopy sets a few Characteristics flags as well - I'm only setting one of them as the other ones are documented to be deprecated. I'd like to have @rnk's opinion on this part.

My suggestion would be to continue setting the deprecated characteristics flags for --strip-all-gnu, but not for --strip-all, if I understand the meaning of the difference between the two flags. --strip-all-gnu should be for more extreme compatibility between gnu, --strip-all is allowed to be a bit more aggressive.
But I have never heard of characteristics before, maybe this is a case where we want to break GNU objcopy compatibility (we don't aim to be bit-for-bit compatible; even GNU-matching flags will different in minor aspects).
Probably best to let @rnk comment on characteristics.

Sounds sensible, but let's wait for his opinion.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

41

I haven't seen that one, thanks!

So if I understand it properly, GNU objcopy earlier set the "relocations stripped" flag if there were no base relocs (the other kind of relocs, on executables/DLLs) in the output binary, but the fix changed it to not do this if there were none in the input to begin with.

So far in these patches, I'm not touching base relocs at all though.

While waiting for @rnk's sentiment, I think these flags aren't strictly necessary wrt this patch at all, but I'm mostly setting them to be helpful.

43

I haven't studied the GNU objcopy output closely enough wrt those stripping options, but I'll probably see that once I finish the patches about stripping sections and debug info. (I have it mostly functional since a long time, but it's missing finalizing according to earlier review comments and making more precise tests for each feature.)

tools/llvm-objcopy/COFF/Writer.cpp

55

Ok, will change.

[mstorsjo]

Using a ternary operator as suggested.

[mstorsjo]

Alternatively, I could just skip setting the characteristics flags altogether in this patch, as I don't think anything actually reads them on object files, and the context here (removing relocations) only is relevant for object files - base relocs on executables/DLLs are untouched. We can always add them later if deemed necessary. WDYT?

[mstorsjo]

I came to the conclusion that setting the Characteristics flags is mostly orthogonal to the stripping of the symbols/relocs (the flag discussed before is actually only relevant for whether executables/DLLs are loadable at a different address than the default), so I split out that part of the patch and I might revisit it later, but it's not essential for the functionality of this patch.

The remainder has been LGTM'd by @jhenderson before, so unless there's other objections I guess that can be pushed at the beginning of next week.

[jhenderson]

Latest version LGTM, but I'd like someone else to give a thumbs up too, as I don't know enough about COFF to be able to spot any potential issues necessarily.

[smeenai]

I don't see anything particularly COFF-specific here ... there were discussions about characteristics flags before, but that doesn't seem to be around in the latest version of the patch, and Martin's reasoning for that (that we're stripping relocations from object files, but the characteristics pertain to base relocations being stripped from image files, which aren't touched by this patch) makes sense. LGTM from that perspective.

[rnk]

lgtm

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

+# RUN: yaml2obj %s > %t.in.o
+
+# RUN: llvm-objdump -t %t.in.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-PRE
+
+# RUN: llvm-objcopy --strip-all %t.in.o %t.out.o
+# RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefix=SYMBOLS
+# RUN: llvm-readobj -file-headers -relocs %t.out.o | FileCheck %s --check-prefixes=CHARS,RELOCS
+
+# Test that -S, llvm-strip without arguments and --strip-out-gnu produces
+# identical output.
+# RUN: llvm-objcopy -S %t.in.o %t.out-short.o
+# RUN: cmp %t.out.o %t.out-short.o
+
+# RUN: cp %t.in.o %t.out-strip.o
+# RUN: llvm-strip %t.out-strip.o
+# RUN: cmp %t.out.o %t.out-strip.o
+
+# RUN: llvm-objcopy --strip-all-gnu %t.in.o %t.out-gnu.o
+# RUN: cmp %t.out.o %t.out-gnu.o
+
+# SYMBOLS: SYMBOL TABLE:
+# SYMBOLS-PRE-NEXT: external
+# SYMBOLS-PRE-NEXT: external_undefined
+# SYMBOLS-EMPTY:
+
+# CHARS:       Characteristics [
+# CHARS-NEXT:    IMAGE_FILE_RELOCS_STRIPPED
+# CHARS-NEXT:  ]
+
+# RELOCS:      Relocations [
+# RELOCS-NEXT: ]
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     488B0500000000C3
+    Relocations:     
+      - VirtualAddress:  3
+        SymbolName:      external_undefined
+        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
+...

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+  // StripAll removes all symbols and thus also removes all relocations.
+  if (Config.StripAll || Config.StripAllGNU) {
+    for (Section &Sec : Obj.Sections)
+      Sec.Relocs.clear();
+    // The description of IMAGE_FILE_RELOCS_STRIPPED only talks about its
+    // role on executables, where it would indicate absence of base relocs,
+    // which are a different thing than the normal COFF relocs (which only
+    // are used in object files). GNU objcopy sets this flag, and it's
+    // probably useful to indicate that the object file is severely
+    // broken by this operation.
+    Obj.CoffFileHeader.Characteristics |= IMAGE_FILE_RELOCS_STRIPPED;
+    // GNU objcopy also sets IMAGE_FILE_LOCAL_SYMS_STRIPPED
+    // (and IMAGE_FILE_LINE_NUMS_STRIPPED), but there's less point in setting
+    // those.
+  }
+
   // If we need to do per-symbol removals, initialize the Referenced field.
   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) {
+    // For StripAll, all relocations have been stripped and we remove all
+    // symbols.
+    if (Config.StripAll || Config.StripAllGNU)
+      return true;
+
     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));

tools/llvm-objcopy/COFF/Writer.cpp

 }
 
 void COFFWriter::layoutSections() {
   for (auto &S : Obj.Sections) {
     if (S.Header.SizeOfRawData > 0)
       S.Header.PointerToRawData = FileSize;
     FileSize += S.Header.SizeOfRawData; // For executables, this is already
                                         // aligned to FileAlignment.
+    S.Header.NumberOfRelocations = S.Relocs.size();
     if (S.Header.NumberOfRelocations > 0)
       S.Header.PointerToRelocations = FileSize;
+    else
+      S.Header.PointerToRelocations = 0;
     FileSize += S.Relocs.size() * sizeof(coff_relocation);
     FileSize = alignTo(FileSize, FileAlignment);
 
     if (S.Header.Characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA)
       SizeOfInitializedData += S.Header.SizeOfRawData;
   }
 }
 
Error COFFWriter::finalize(bool IsBigObj) {
 
   size_t PointerToSymbolTable = FileSize;
   // StrTabSize <= 4 is the size of an empty string table, only consisting
   // of the length field.
   if (SymTabSize == 0 && StrTabSize <= 4 && Obj.IsPE) {
     // For executables, don't point to the symbol table and skip writing
     // the length field, if both the symbol and string tables are empty.
     PointerToSymbolTable = 0;
     StrTabSize = 0;
   }
 
   size_t NumRawSymbols = SymTabSize / SymbolSize;
   Obj.CoffFileHeader.PointerToSymbolTable = PointerToSymbolTable;
   Obj.CoffFileHeader.NumberOfSymbols = NumRawSymbols;
   FileSize += SymTabSize + StrTabSize;
   FileSize = alignTo(FileSize, FileAlignment);