[llvm-strip] Add support for -remove-section

[alexander-shaposhnikov]

Summary

This diff adds support for -remove-section.
Test plan: make check-all

[jakehehrlich]

test/tools/llvm-objcopy/strip-and-remove.test

1

Why not just extend the existing --remove-section tests with llvm-strip?

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

1

the existing tests

alexshap-mbp:llvm-objcopy alexshap$ grep -n remove-section ./*

./armexidx-link.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
./group-big-endian.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
./group-unchanged.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
./group.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2

those tests verify something else and the inputs don't contain debug info.
At the beginning i thought i would extend one of them,
but then it turned out to be less straightforward than I expected or I don't see the test you are referring to.

[jhenderson]

test/tools/llvm-objcopy/strip-and-remove.test

1

There are a few tests like remove-section.test that only use the -R short option. It might be better to extend those (and probably also test both short and long options to avoid this confusion).

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

1

oh, right, will do

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

1

right now I'm looking at remove-section.test - i can try to extend (essentially replace) that test, but to be honest I'm not 100% sure it's a good idea since that test was probably designed to be simple and verify -remove-section basic functionality only
(in particular, it doesn't have debuginfo, symbols etc).

@jakehehrlich - what are your thoughts - are you sure it's a good idea to load that test with all this stuff ?

looking at my diff - it seems to me that it would be better to merge strip-and-remove.test and strip-debug-and-remove.test since they share the "input", what would you say to this approach ? (either way, i don't have a strong opinion on this)

[jakehehrlich]

test/tools/llvm-objcopy/strip-and-remove.test

1

So thinking about this, the complication seems to be that -R dosn't disable --strip-all so it dosn't fit neatly into the existing test. In light of that I don't suppose it's all that easy to add that functionality. That makes me think we might want a --no-strip option for testing's sake since we want to add lots of 'cmp' checks to our tests to make sure llvm-strip and llvm-objcopy never differ by even a byte.

I don't care much about dedupping yaml TBH but I do care about test failures being as meaningful as possible (which means airing on the side of not merging). I do care about testing that llvm-objcopy and llvm-strip are binary equivalent however. I'd be satisfied with just adding cmp checks to these existing checks. I think the llvm-objcopy invocations will also clarify a) why this test is separate and b) will document the semantics of llvm-strip.

Also adding both short and long versions (as James recommended) and running cmp to make sure those are the same would be nice. Basically if there are multiple ways of doing the *exact* same thing I want to test all those to make sure they are in fact exactly the same to ensure that nothing ever diverges.

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

1

sorry, not sure i understand why we would need a new flag "--no-strip", maybe i'm missing smth though, as i said above - to test strip the input (to make the test meaningful) should have debug info and symbols, the existing test remove-section.test at the moment contains neither of them (debug info nor symbols) and I'm hesitating to load that particular test with complications.

For example, in strip-debug-and-remove.test:

llvm-strip --strip-debug -remove-section=.text.bar a.o

is equivalent to

llvm-objcopy --strip-debug -remove-section=.text.bar b.o b.o

cmp a.o b.o

i can add objcopy /cmp invocations to these tests - is that what you are saying / sounds good or there are some other concerns i've missed ?

[jakehehrlich]

test/tools/llvm-objcopy/strip-and-remove.test

1

--no-strip would be an option for llvm-strip that disabled --strip-all without enabling another strip option. I don't think it should be added in this test and I don't think you should touch remove-sections.test. It's just an option I might add for testing.

As for the second part, yes, that's what I mean and that covers my concerns.

[alexander-shaposhnikov]

Address comments

[alexander-shaposhnikov]

Remove blank line

[jakehehrlich]

test/tools/llvm-objcopy/strip-and-remove.test

2–3

I'd prefer not regenerating the elf. You don't even need to run the FileCheck again. Principally this is an llvm-strip test and we just want to add a couple lines to make sure the equivlient llvm-objcopy produces an exactly equivalent binary. It is assumed that if the binaries are equivalent they will produce equivalent results.

[alexander-shaposhnikov]

generate elf only once

[alexander-shaposhnikov]

Fix typo

[jakehehrlich]

LGTM: Please wait on James as I'd like his input on how we should test this sort of stuff going forward as well.

test/tools/llvm-objcopy/remove-section.test

2 ↗

(On Diff #145784)

nit: I'd prefer not to copy in this case

[alexander-shaposhnikov]

test/tools/llvm-objcopy/remove-section.test

2 ↗

(On Diff #145784)

i copied because potentially llvm-objcopy can modify the input (i.e. by mistake),
it's not the case here, but my motivation was to reduce the room for potential interfering of different runs.

[jakehehrlich]

test/tools/llvm-objcopy/remove-section.test

2 ↗

(On Diff #145784)

Ah, in that case can you add a check after the first objcopy to check that %t and %t1 are binary equivalent after the first call to objcopy? That way we can detect that case if it occurs.

[alexander-shaposhnikov]

Add one more "defensive" cmp

[rja]

In D46567#1092021, @alexshap wrote:

Add one more "defensive" cmp

LG

[jhenderson]

The overall approach looks fine to me. I think I agree with your comments about not overloading remove-section.test. I think your solution is actually better in this case. We don't have much combination testing of switches in llvm-objcopy, if I remember correctly, so having distinct llvm-strip tests where we are effectively testing some new combo is reasonable, in my opinion, but as noted by @jakehehrlich, we should definitely check the equivalent llvm-objcopy equivalent in the same test. Essentially if we can do some invocation of llvm-objcopy and some (potentially different) invocation of llvm-strip and get the same output in both cases, there should be a test containing them both. If the behaviour cannot be replicated by llvm-strip, then we should only test llvm-objcopy. That being said, I'm not opposed to a --no-strip switch, although I'm not as certain of the value.

LGTM, with lots of small comments, though I'm not too bothered by any of them, so if you disagree with any, feel free to go ahead and commit anyway.

test/tools/llvm-objcopy/remove-section.test

1 ↗

(On Diff #145793)

No issues with the changes made in this test, but since this isn't testing llvm-strip, I'd prefer it if they were in a separate commit. Feel free to commit it without further review.

6 ↗

(On Diff #145793)

Probably worth commenting why we do a check here. There are probably several other cases in other tests where we do something similar too, which should probably be updated.

test/tools/llvm-objcopy/strip-and-remove.test

2

What are your thoughts on renaming this test to strip-all-and-remove.test? That way it's clear that we are testing the combination of the two switches of llvm-objcopy, one of which is effectively always on for llvm-strip. It also allows for easier distinguishing from strip-debug.

18

If this is intended to be a real debug section, it probably is worth naming it in the same style as other debug sections i.e. .debug_foo.

45

Should we have a CHECK-NOT: Name: .symtab here?

Strictly speaking, an empty symbol table is distinct from a missing one.

test/tools/llvm-objcopy/strip-debug-and-remove.test

18

See my comments in the other test.

[alexander-shaposhnikov]

test/tools/llvm-objcopy/remove-section.test

1 ↗

(On Diff #145793)

r332078

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

2

yeah, sounds good to me, will rename this test to strip-all-and-remove.test

18

ok, will rename it to .debug_foo

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-and-remove.test

45

yeah

test/tools/llvm-objcopy/strip-and-remove.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-strip -remove-section=.text.bar %t
+# RUN: llvm-readobj -file-headers -sections -symbols %t | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .debugfoo
+    Type:            SHT_PROGBITS
+    Content:         "00000000"
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+  - Name:            .text.bar
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+Symbols:
+  Global:
+    - Name: debugfoo
+      Section: .debugfoo
+    - Name: foo
+      Section: .text
+    - Name: bar
+      Section: .text.bar
+ 
+# CHECK: SectionHeaderCount: 3
+
+# CHECK:     Name: .text
+# CHECK:     Name: .shstrtab
+# CHECK-NOT: Name: .text.bar
+# CHECK-NOT: Name: .debugfoo
+
+# Check that *only* foo is copied and no debugfoo or textbar
+# CHECK:      Symbols [
+# CHECK-NEXT: ]

test/tools/llvm-objcopy/strip-debug-and-remove.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-strip -strip-debug -remove-section=.text.bar %t
+# RUN: llvm-readobj -file-headers -sections -symbols %t | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .debugfoo
+    Type:            SHT_PROGBITS
+    Content:         "00000000"
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+  - Name:            .text.bar
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+Symbols:
+  Global:
+    - Name: debugfoo
+      Section: .debugfoo
+    - Name: foo
+      Section: .text
+    - Name: bar
+      Section: .text.bar
+ 
+# CHECK: SectionHeaderCount: 5
+
+# CHECK:     Name: .text
+# CHECK:     Name: .symtab
+# CHECK:     Name: .strtab
+# CHECK:     Name: .shstrtab
+# CHECK-NOT: Name: .text.bar
+# CHECK-NOT: Name: .debugfoo
+
+# Check that *only* foo is copied and no debugfoo or textbar
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Binding:
+# CHECK-NEXT:     Type:
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:     Section: Undefined
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: foo
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Binding: Global
+# CHECK-NEXT:     Type:
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]

tools/llvm-objcopy/StripOpts.td

 include "llvm/Option/OptParser.td"
 
 multiclass Eq<string name> {
   def NAME: Separate<["--", "-"], name>;
   def NAME # _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
 }
 
 def help : Flag<["-", "--"], "help">;
 
 def strip_debug : Flag<["-", "--"], "strip-debug">,
                   HelpText<"Remove debugging symbols only">;
 
+defm remove_section : Eq<"remove-section">,
+                      MetaVarName<"section">,
+                      HelpText<"Remove <section>">;
+
+def R : JoinedOrSeparate<["-"], "R">,
+        Alias<remove_section>;

tools/llvm-objcopy/llvm-objcopy.cpp

CopyConfig ParseStripOptions(ArrayRef<const char *> ArgsArr) {
   CopyConfig Config;
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[0];
 
   // Strip debug info only.
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
   if (!Config.StripDebug)
     Config.StripAll = true;
+  
+  for (auto Arg : InputArgs.filtered(STRIP_remove_section))
+    Config.ToRemove.push_back(Arg->getValue());
+  
   return Config;
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
   CopyConfig Config;
   if (sys::path::stem(ToolName).endswith_lower("strip"))
     Config = ParseStripOptions(makeArrayRef(argv + 1, argc));
   else
     Config = ParseObjcopyOptions(makeArrayRef(argv + 1, argc));
   ExecuteElfObjcopy(Config);
 }