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

Fri Jan 11 14:07:21 PST 2019

rupprecht added a comment.

> 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.

================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:41
+    // broken by this operation.
+    Obj.CoffFileHeader.Characteristics |= IMAGE_FILE_RELOCS_STRIPPED;
+    // GNU objcopy also sets IMAGE_FILE_LOCAL_SYMS_STRIPPED
----------------
It looks like this is not always supposed to be added; have you seen https://sourceware.org/bugzilla/show_bug.cgi?id=11396?

================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:43
+    // GNU objcopy also sets IMAGE_FILE_LOCAL_SYMS_STRIPPED
+    // (and IMAGE_FILE_LINE_NUMS_STRIPPED), but there's less point in setting
+    // those.
----------------
What about `IMAGE_FILE_DEBUG_STRIPPED`? Do you know if GNU objcopy sets that in any cases?

================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:55
+    else
+      S.Header.PointerToRelocations = 0;
     FileSize += S.Relocs.size() * sizeof(coff_relocation);
----------------
IMO this if/else block is actually easier to read with a ternary:

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

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

https://reviews.llvm.org/D56481