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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 3 inline comments as done.
mstorsjo added a comment.

In D56481#1354845 <https://reviews.llvm.org/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.



================
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
----------------
rupprecht wrote:
> It looks like this is not always supposed to be added; have you seen https://sourceware.org/bugzilla/show_bug.cgi?id=11396?
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.


================
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.
----------------
rupprecht wrote:
> What about `IMAGE_FILE_DEBUG_STRIPPED`? Do you know if GNU objcopy sets that in any cases?
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.)


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


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

https://reviews.llvm.org/D56481