Page MenuHomePhabricator

[llvm-strip] Support --strip-sections with the llvm-strip command.
ClosedPublic

Authored by wolfgangp on Aug 5 2019, 6:19 PM.

Details

Summary

llvm-objcopy supports --strip-sections, while its alias llvm-strip does not. The option seems like a good fit for llvm-strip, so this patch adds support for it.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp created this revision.Aug 5 2019, 6:19 PM

@jakehehrlich/@rupprecht, are you aware of any reason why we SHOULDN'T have this in llvm-strip? It seems odd to have it in llvm-objcopy when the original source was eu-strip!

llvm/test/tools/llvm-objcopy/ELF/strip-sections.test
15 ↗(On Diff #213508)

Better would be to do a cmp of %t2 and %t4. The two outputs should be byte-for-byte identical.

I am on the fence. On one side, for extensions that binutils does not support, I think it is fine to just support them in llvm-objcopy, not in llvm-strip. On the other side, --strip-sections is a strip-* option, so it isn't a bad fit for llvm-strip.

Links: --strip-sections was implemented by D38335. In eu-strip, it was implemented by https://sourceware.org/git/?p=elfutils.git;a=commit;h=0b9d1fb534604a9ba19999cd8ce8e7efce28da24

I am on the fence. On one side, for extensions that binutils does not support, I think it is fine to just support them in llvm-objcopy, not in llvm-strip. On the other side, --strip-sections is a strip-* option, so it isn't a bad fit for llvm-strip.

We have a concrete use-case for this internally, so I'd like to have it if there isn't a good reason not to.

I am on the fence. On one side, for extensions that binutils does not support, I think it is fine to just support them in llvm-objcopy, not in llvm-strip. On the other side, --strip-sections is a strip-* option, so it isn't a bad fit for llvm-strip.

We have a concrete use-case for this internally, so I'd like to have it if there isn't a good reason not to.

Then add this option.

The description should be changed to state that this is a strip option, so it seems a good fit for llvm-strip. We already have --strip-all --strip-all-gnu --strip-debug --strip-unneeded --strip-symbol so it seems natural to have another --strip-*.

The reasons for this are unclear, so this patch adds the option to llvm-strip.

I'm a bit unclear on what this means:

The description should be changed to state that this is a strip option, so it seems a good fit for llvm-strip. We already have --strip-all --strip-all-gnu --strip-debug --strip-unneeded --strip-symbol so it seems natural to have another --strip-*.

What description should be changed? The help text? Right now I see few distinctions between the 2 invocations in the *.td files for objcopy and strip respectively.

I'm a bit unclear on what this means:

The description should be changed to state that this is a strip option, so it seems a good fit for llvm-strip. We already have --strip-all --strip-all-gnu --strip-debug --strip-unneeded --strip-symbol so it seems natural to have another --strip-*.

What description should be changed? The help text? Right now I see few distinctions between the 2 invocations in the *.td files for objcopy and strip respectively.

This sentence in the description:

The reasons for this are unclear, so this patch adds the option to llvm-strip.

We do not add options merely for "unclear" reasons. It is because the option is a good fit for llvm-strip.

CC'ing mmpozulp due to him writing the llvm-strip doc in D65384. This option will need adding, assuming this patch lands first.

This sentence in the description:

The reasons for this are unclear, so this patch adds the option to llvm-strip.

We do not add options merely for "unclear" reasons. It is because the option is a good fit for llvm-strip.

Well, of course! The unclear part was why it wasn't already supported in the first place. So I see we're in violent agreement.

wolfgangp edited the summary of this revision. (Show Details)Aug 7 2019, 10:39 AM
wolfgangp updated this revision to Diff 213948.Aug 7 2019, 10:41 AM
wolfgangp marked an inline comment as done.

Addressed review comments:

Use simple file comparison instead of performing the whole test again to verify support for --strip-sections with llvm-strip.

rupprecht accepted this revision.Aug 7 2019, 11:48 AM

@jakehehrlich/@rupprecht, are you aware of any reason why we SHOULDN'T have this in llvm-strip? It seems odd to have it in llvm-objcopy when the original source was eu-strip!

No, if we have the option already implemented, it should be available to both tools.

llvm/test/tools/llvm-objcopy/ELF/strip-sections.test
13–15 ↗(On Diff #213948)

You can create a separate output file with -o

# RUN: llvm-strip --strip-sections %t -o %t4
# RUN: cmp %t2 %t4
This revision is now accepted and ready to land.Aug 7 2019, 11:48 AM
This revision was automatically updated to reflect the committed changes.
wolfgangp marked an inline comment as done.