[llvm-objcopy] Add --keep-symbol (-K) option

[paulsemel]

Summary

This option permits to explicitly keep the specified symbol so that it doesn't get removed.

[jhenderson]

Code change looks fine to me. However, what is the GNU objcopy behaviour for doing --strip-symbol/--keep-symbol in the same command-line? Does last win, or is the symbol always kept, or is the symbol always discarded? Please also add a test for our behaviour for this pair of switches, as it is an interesting edge case.

Also, I know that at this stage the behaviour of --keep-symbol doesn't affect symbols in discarded sections (e.g. due to --remove-section), but could you add a test for attempting to keep a symbol in such a section anyway, please, to document the behaviour, in case we decide to refactor things later.

[paulsemel]

In D46819#1097538, @jhenderson wrote:

Code change looks fine to me. However, what is the GNU objcopy behaviour for doing --strip-symbol/--keep-symbol in the same command-line? Does last win, or is the symbol always kept, or is the symbol always discarded? Please also add a test for our behaviour for this pair of switches, as it is an interesting edge case.

So, for --strip-symbol as for --discard options, the --keep-symbol option win on the previous ones, that's why I didn't add a second test, but I can do it for sure :)

Also, I know that at this stage the behaviour of --keep-symbol doesn't affect symbols in discarded sections (e.g. due to --remove-section), but could you add a test for attempting to keep a symbol in such a section anyway, please, to document the behaviour, in case we decide to refactor things later.

Sure, I will add a test for this behavior !

[paulsemel]

Added some tests for :

• in addition with --remove-section
• in addition with the --strip-symbol option

[jhenderson]

LGTM with the suggested comments.

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

2 ↗

(On Diff #146573)

Nit: I'd suggest using only long options in this sort of test, to make it a little clearer.

test/tools/llvm-objcopy/keep-symbol.test

3 ↗

(On Diff #146573)

Could you make this "-K foo -N foo -N bar --keep-symbol bar -N baz" or similar, to show that order doesn't matter, please?

[paulsemel]

Applied James' nits

[paulsemel]

I'll wait for @jakehehrlich approval to land this one :)

[alexander-shaposhnikov]

Would be great to expose this option in llvm-strip, not necessarily in this
patch, though.

[jakehehrlich]

LGTM

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

2 ↗

(On Diff #146573)

I'd like to +1. It's good to test that the short options work but I can never remember what the aliases are. I had to go look them up for keep-symbol.test

test/tools/llvm-objcopy/keep-symbol.test

2 ↗

(On Diff #146585)

nit: Can you use --discard-all following James recommendation above. Since --discard-all isn't being tested (nor any related option really) here it shouldn't use an alias.

[alexander-shaposhnikov]

btw, i've looked at the code one more time, it seems to me it's not completely correct, the situation is a bit more subtle.
For example,

main.c:
int xx= 1;
int yy() { return 239; }

gcc -g -c main.c -o main.o
cp main.o _main.o

objcopy --keep-symbol xx --strip-all main.o -o main2.o
llvm-objcopy --keep-symbol xx --strip-all _main.o -o _main2.o

llvm-objcopy will entirely wipe out the symbols table, the logic appears to be wrong
(bintuils objcopy does the right thing at the same time)

[alexander-shaposhnikov]

yeah, the behavior of --keep-symbol is more complicated than what's going on here.
In particular,

main.c

attribute((section(".data.foo"))) int xx= 1;
int yy() { return 239; }

objcopy --keep-symbol xx --remove-section .data.foo main.o main2.o

objcopy: main1.o: symbol `xx' required but not present
objcopy:main1.o: No symbols
[alexshap@devvm ~/local] echo $?
1

so probably the exit code should not be 0, error reporting can be better, though, than what binutils objcopy has to offer.

@paulsemel , can you send fix (looks like it's not that involved to fix it) ? otherwise i can send a patch for review myself . cc: @jakehehrlich

[jakehehrlich]

In D46819#1100415, @alexshap wrote:

yeah, the behavior of --keep-symbol is more complicated than what's going on here.
In particular,

main.c

attribute((section(".data.foo"))) int xx= 1;
int yy() { return 239; }

objcopy --keep-symbol xx --remove-section .data.foo main.o main2.o

objcopy: main1.o: symbol `xx' required but not present
objcopy:main1.o: No symbols
[alexshap@devvm ~/local] echo $?
1

so probably the exit code should not be 0, error reporting can be better, though, than what binutils objcopy has to offer.

@paulsemel , can you send fix (looks like it's not that involved to fix it) ? otherwise i can send a patch for review myself . cc: @jakehehrlich

I'm not sure how concerned I am about this but I've got some thoughts I suppose. The issue here is that GNU objcopy implicitly keeps a section if a symbol is explicitly kept that is defined in that section. Right now we don't keep such sections. I'm fine with the added error for --remove-section and --keep-symbol conflicts because those are explicit but I don't think we should error out for all such cases and I don't want to put any extra effort into erroring out in that case. In more complicated mixtures of symbol keeping and section removals we should probably err on the side of caution by not removing sections (which appears to be what GNU objcopy does as well). This is probably a slightly non-trivial feature to implement. The reasoning is that in some big build success is more important than reducing size. A warning should be issued at best in that case and I'd only consider an error valid in the case of an explicit-keep explicit-remove conflict. In past discussions of this form it was decided that we shouldn't worry about that sort of error. In general an explict-keep should override *everything*.

So my recommendation would be to implement keeping of sections for kept symbols defined in those sections but not to worry about the error case. In fact I'm not even sure I'd be ok with an error in that case.

[alexander-shaposhnikov]

@jakehehrlich Jake, it's one issue, a separate issue (which is probably more important and creating troubles right now when i first tried to use this newly-implemented flag) (mentioned in my first comment) is
that llvm-objcopy removes the symbols table regardless of -K (for a detailed example have a look at that comment), imo this needs to be fixed. Am I missing smth ?

[alexander-shaposhnikov]

cc: @jhenderson

[jhenderson]

In D46819#1100483, @alexshap wrote:

@jakehehrlich Jake, it's one issue, a separate issue (which is probably more important and creating troubles right now when i first tried to use this newly-implemented flag) (mentioned in my first comment) is
that llvm-objcopy removes the symbols table regardless of -K (for a detailed example have a look at that comment), imo this needs to be fixed. Am I missing smth ?

I've not looked at the GNU behaviour, but if GNU keeps that symbol (and only that symbol), it implies that we cannot simply strip the symbol table section via the removeSections code. Instead I'd suggest we need to do two things: 1) If Config.StripAll is set, remove a symbol, unless it is marked as keep. 2) If Config.StripAll is set, and the symbol table is empty (i.e. no symbols were marked as keep), we strip the symbol table.

I think --strip-all is an example of an "implicit" removal, rather than explicit, so the explicit keep should win out with no warning or error.

[paulsemel]

Hi @alexshap !

I actually found this behavior yesterday, but I don't think the problem resides in this option implementation, but more in option conflicts (I dive deep in objcopy source code, and it seems that they have such conflict management).
So, I think you're right and we might do something here, but I truly think that it might not be a fix of this option, but more a real option conflicts management thing.
As far as I remember during my tests, this is not the only case where we have options conflicts now.
What do you think ?
cc @jakehehrlich @jhenderson

[alexander-shaposhnikov]

@paulsemel, as i said, there are 2 (similar but separate) issue.
I'm mostly concerned with the fact that we incorrectly remove the symbols table if --strip-all -K foo is specified because it's a real world use-case.
As @jhenderson suggested

>Instead I'd suggest we need to do two things: 1) If Config.StripAll is set, remove a symbol, unless it is marked as keep. 2) If Config.StripAll is set, and the symbol table is empty (i.e. no symbols were marked as 
>keep), we strip the symbol table.

i think this is how it should work, so i completely agree with James.
Does it sound good to you ?

[paulsemel]

In D46819#1101523, @alexshap wrote:

@paulsemel, as i said, there are 2 (similar but separate) issue.
I'm mostly concerned with the fact that we incorrectly remove the symbols table if --strip-all -K foo is specified because it's a real world use-case.
As @jhenderson suggested

>Instead I'd suggest we need to do two things: 1) If Config.StripAll is set, remove a symbol, unless it is marked as keep. 2) If Config.StripAll is set, and the symbol table is empty (i.e. no symbols were marked as 
>keep), we strip the symbol table.

i think this is how it should work, so i completely agree with James.
Does it sound good to you ?

Sure, totally fine with this. My point was more that the problem was not in this patch (I mean, even if you would have find this case before, this would still have been a separate one, as it more concerns the --strip-all option implementation than this one).
Anyway, I think that the current architecture is not really compliant with this (maybe the symbol removal should happen before the section removal ?)

[paulsemel]

! In D46819#1101534, @paulsemel wrote:
Sure, totally fine with this. My point was more that the problem was not in this patch (I mean, even if you would have find this case before, this would still have been a separate one, as it more concerns the --strip-all option implementation than this one).
Anyway, I think that the current architecture is not really compliant with this (maybe the symbol removal should happen before the section removal ?)

No, forget about it, we should probably do another pass after symbol removal.

[alexander-shaposhnikov]

to avoid misunderstanding - how will we proceed ? (should i send a diff myself or you will fix it)

[paulsemel]

In D46819#1101548, @alexshap wrote:

to avoid misunderstanding - how will we proceed ? (should i send a diff myself or you will fix it)

You seem to have something clear in mind, so you can go for it, I don't matter :)

llvm/trunk/test/tools/llvm-objcopy/keep-symbol-remove-section.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --remove-section .text --keep-symbol foo %t %t2
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
+Symbols:
+  Local:
+    - Name:     foo
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+
+#CHECK: Symbols [
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name:
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

llvm/trunk/test/tools/llvm-objcopy/keep-symbol.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --discard-all -K foo --keep-symbol bar %t %t2
+# RUN: llvm-objcopy -K foo -N foo -N bar --keep-symbol bar -N baz %t %t3
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s
+# RUN: llvm-readobj -symbols %t3 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
+Symbols:
+  Local:
+    - Name:     foo
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+    - Name:     bar
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1008
+      Size:     8
+    - Name:     baz
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1010
+      Size:     8
+
+#CHECK: Symbols [
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name:
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: foo
+#CHECK-NEXT:    Value: 0x1000
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: bar
+#CHECK-NEXT:    Value: 0x1008
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

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

def discard_all : Flag<["-", "--"], "discard-all">,
                       HelpText<"Remove all local symbols except file and section symbols">;
 def x : Flag<["-"], "x">,
         Alias<discard_all>;
 defm strip_symbol : Eq<"strip-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Remove symbol <symbol>">;
 def N : JoinedOrSeparate<["-"], "N">,
         Alias<strip_symbol>;
+defm keep_symbol : Eq<"keep-symbol">,
+                       MetaVarName<"symbol">,
+                       HelpText<"Do not remove symbol <symbol>">;
+def K : JoinedOrSeparate<["-"], "K">,
+        Alias<keep_symbol>;

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

struct CopyConfig {
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
   std::vector<StringRef> SymbolsToLocalize;
   std::vector<StringRef> SymbolsToGlobalize;
   std::vector<StringRef> SymbolsToWeaken;
   std::vector<StringRef> SymbolsToRemove;
+  std::vector<StringRef> SymbolsToKeep;
   StringMap<StringRef> SymbolsToRename;
   bool StripAll = false;
   bool StripAllGNU = false;
   bool StripDebug = false;
   bool StripSections = false;
   bool StripNonAlloc = false;
   bool StripDWO = false;
   bool ExtractDWO = false;
Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
         Sym.Binding = STB_WEAK;
 
       const auto I = Config.SymbolsToRename.find(Sym.Name);
       if (I != Config.SymbolsToRename.end())
         Sym.Name = I->getValue();
     });
 
     Obj.removeSymbols([&](const Symbol &Sym) {
+      if (!Config.SymbolsToKeep.empty() &&
+          is_contained(Config.SymbolsToKeep, Sym.Name))
+        return false;
+
       if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
           Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
           Sym.Type != STT_SECTION)
         return true;
 
       if (!Config.SymbolsToRemove.empty() &&
           is_contained(Config.SymbolsToRemove, Sym.Name)) {
         return true;
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());
+  for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
+    Config.SymbolsToKeep.push_back(Arg->getValue());
 
   return Config;
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
 CopyConfig ParseStripOptions(ArrayRef<const char *> ArgsArr) {