In D56979#1364855, @EricWF wrote:

Does libc++ need a similar change?

Updated to test how --only-section combined with --strip-debug behaves - matching GNU objcopy's behaviour.

In D56873#1363068, @jhenderson wrote:

Just to confirm, the desired behaviour of combining certain switches (e.g. --strip-debug and --only-section) is for the explicitly mentioned --only-section section to be kept despite it potentially being removed by the other switch?

@rnk or @smeenai - This is awaiting an ack from a COFF perspective.

Applied @jhenderson's suggestions.

Adding another COFF-knowledgeable reviewer

Applied @jhenderson's suggestions. But I also split the input data into a separate yaml file in Inputs, as I'm going to use the same input for another test.

Fixed @jhenderson's suggestions.

In D56683#1360543, @rupprecht wrote:

It'd be nice if someone else could take a pass too, especially for any COFF-specific aspects, but LGTM

Tweaked the scope of lambdas in removeSections to avoid use-after-free of stack variables in the implementation of the lambda captures, as pointed out by asan.

Applied @rupprecht's suggestions.

Using a lambda that takes StringRef, renamed the lambda parameter to match coding conventions. Will commit tomorrow.

In D56659#1356023, @jhenderson wrote:

LGTM. I'm assuming that the -1 SectionNumber is what makes them absolute?

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.

LGTM

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?

Using a ternary operator as suggested.

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.

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

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

In D56584#1353961, @jhenderson wrote:

In D56584#1353933, @mstorsjo wrote:

In D56584#1353916, @jhenderson wrote:

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

Initially I tried to make the output of llvm-objcopy as close as possible to the output from various tools (llvm-mc and MSVC), and to achieve this, I only skipped including the symbol/string table in executables, while keeping it present in object files.

I know that exactly matching the output of another tool shouldn't be a goal in itself, but I don't know if there are tools out there that would flat out reject an object file with an omitted symbol/string table, as opposed to an empty symbol/string table. So keeping this distinction probably is safer.

So the behaviour change is that for objects, the object now has a pointer to the symbol table, which it didn't before. I think I get it now. The problem is the writeSymbolStringTables function, and you want that to always write a size for object files, even if the table is empty, right?

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

In D56584#1353916, @jhenderson wrote:

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

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

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