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

[wolfgangp]

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.

[jhenderson]

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

[MaskRay]

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

[jhenderson]

In D65787#1616681, @MaskRay wrote:

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.

[MaskRay]

In D65787#1616703, @jhenderson wrote:

In D65787#1616681, @MaskRay wrote:

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.

[wolfgangp]

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.

[MaskRay]

In D65787#1617372, @wolfgangp wrote:

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.

[jhenderson]

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

[wolfgangp]

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]

Addressed review comments:

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

[rupprecht]

In D65787#1616420, @jhenderson wrote:

@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

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-sections.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --strip-sections %t %t2
 # RUN: llvm-readobj --file-headers --program-headers %t2 | FileCheck %s
 # RUN: od -t x1 -j 4096 -N 12 %t2 | FileCheck %s --check-prefix=DATA
 
 ## Sanity check the DATA-NOT line by showing that "fe ed fa ce" appears
 ## if --strip-sections is not specified.
 # RUN: llvm-objcopy %t %t3
 # RUN: od -t x1 -j 4096 -N 12 %t3 | FileCheck %s --check-prefix=VALIDATE
 
+## Check that llvm-strip --strip-sections is equivalent to
+## llvm-objcopy --strip-sections. 
+# RUN: llvm-strip --strip-sections %t -o %t4
+# RUN: cmp %t2 %t4
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name:            .text

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

parseStripOptions(ArrayRef<const char *> ArgsArr,
   Config.AllowBrokenLinks = InputArgs.hasArg(STRIP_allow_broken_links);
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
 
   if (InputArgs.hasArg(STRIP_discard_all, STRIP_discard_locals))
     Config.DiscardMode =
         InputArgs.hasFlag(STRIP_discard_all, STRIP_discard_locals)
             ? DiscardType::All
             : DiscardType::Locals;
+  Config.StripSections = InputArgs.hasArg(STRIP_strip_sections);
   Config.StripUnneeded = InputArgs.hasArg(STRIP_strip_unneeded);
   if (auto Arg = InputArgs.getLastArg(STRIP_strip_all, STRIP_no_strip_all))
     Config.StripAll = Arg->getOption().getID() == STRIP_strip_all;
   Config.StripAllGNU = InputArgs.hasArg(STRIP_strip_all_gnu);
   Config.OnlyKeepDebug = InputArgs.hasArg(STRIP_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(STRIP_keep_file_symbols);
 
   for (auto Arg : InputArgs.filtered(STRIP_keep_section))

llvm/trunk/tools/llvm-objcopy/StripOpts.td

 
 def strip_all_gnu : Flag<["--"], "strip-all-gnu">,
                     HelpText<"Compatible with GNU strip's --strip-all">;
 def strip_debug : Flag<["--"], "strip-debug">,
                   HelpText<"Remove debugging symbols only">;
 def d : Flag<["-"], "d">, Alias<strip_debug>;
 def g : Flag<["-"], "g">, Alias<strip_debug>;
 def S : Flag<["-"], "S">, Alias<strip_debug>;
+def strip_sections
+    : Flag<["--"], "strip-sections">,
+      HelpText<"Remove all section headers and all sections not in segments">;
 def strip_unneeded : Flag<["--"], "strip-unneeded">,
                      HelpText<"Remove all symbols not needed by relocations">;
 
 defm remove_section : Eq<"remove-section", "Remove <section>">,
                       MetaVarName<"section">;
 def R : JoinedOrSeparate<["-"], "R">, Alias<remove_section>;
 
 defm strip_symbol : Eq<"strip-symbol", "Strip <symbol>">,