[llvm-objcopy] Fix the behavior of --strip-* and --keep-symbol

[alexander-shaposhnikov]

Summary

I've played with binutils objcopy --strip-all --keep-symbol foo,
so if the symbol table contains the symbol "foo"
it's not getting remove.

Test plan: make check-all

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

226 ↗

(On Diff #147438)

If possible I'd prefer this come after and I'd prefer not using the functions in removeSymbols to keep state. I think it's sufficient to just check that SymbolsToKeep is non-empty for the single explicit section keep.

228 ↗

(On Diff #147438)

Not something I want to fix here but I just realized that passing thought the symbol table every time is really inefficient for most use cases. We should somehow change that.

253 ↗

(On Diff #147438)

Also same thing here. Could you add TODOs to only remove symbols when there is an option that effects them?

260 ↗

(On Diff #147438)

I think we can just make Strips imply DiscardAll when a symbol is kept instead of doing this. Alternatively StripAll and StripAllGNU should cause each symbol to be removed and then only remove the symbol table if its empty in those options (which is closer to what GNU objcopy does anyway).

296 ↗

(On Diff #147438)

Instead of duplicating all of these you can add this once at the end as a single keep step

Proposal aggregated from my comments

if (!Config.SymbolsToKeep.empty())
  RemovePred = [RemovePred, &Obj](const SectionBase &Sec] {
    if ( /* section is symbol table or symbol table's string table */)
      return false;
    return RemovePred(Sec);
  }

[paulsemel]

Hi !
I have a few thoughts about option conflicts handling.

First, for example, if we take localize-hidden.test test, and we apply -R .rel.text -N undefLocal, this way we would trigger an error. I know that objcopy is behaving the same as this patch behaves, but I was wondering if we could/want to do better ?

Then, for the example you gave, it seems that the behavior is not matching what you're saying. For instance, still with localize-hidden.test with --strip-all -K defaultGlobal, every other global/weak symbols are kept, which is not the case with objcopy.
Last, if you really test this, you will notice that it won't work, because there will be an error complaining about a symbol referenced by a relocation. This error happens because you are trying to remove undefLocal symbol, which is not permitted while it is a relocatable symbol.

I think this change is not sufficient to handle this problem (and more generally option conflicts), and it is creating other problems we might at least be aware of.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

296 ↗

(On Diff #147438)

that's what i originally did,
it doesn't work correctly, or, at least not what binutils objcopy does.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

260 ↗

(On Diff #147438)

>StripAll and StripAllGNU should cause each symbol to be removed -

• it turns out that if at least one symbol specified by -K is kept, than all the local symbols are kept as well, that's what both objcopy and strip do, originally i didn't notice that and the code was much simpler. Anyway - i'll look into this tomorrow, kinda tired and want to sleep, maybe i sent this diff too quickly, want to think a bit more.

296 ↗

(On Diff #147438)

i will look again tomorrow

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

226 ↗

(On Diff #147438)

@jakehehrich, Jake, so basically binutils objcopy does the following (as i already tried to explain in my previous comment) (if I'm not mistaken or not missing smth): if at least one symbol specified by -K is kept, then all the local symbols are kept as well, and the symbols table / strings table are not getting removed.
So it looks like if we want to repeat this logic (in particular regarding the local symbols), we can either keep track of this state (what's going on right now), or we can scan the symbols beforehand and check if it contains any of the symbols specified by -K.
(checking if the symbols table is empty or not is not sufficient since (as i mentioned above) we are not allowed to remove the local symbols anyway, so it'll be non-empty anyway in this case.
What are your thoughts / preferences ?

[jhenderson]

I might have missed something, but why do we need to keep _all_ of the local symbols, if one is kept? Surely that's completely unnecessary? And why local symbols and not global or weak symbols? It seems pretty straightforward that --strip-all --K <some symbol> should produce a symbol table with exactly one symbol in it (plus the null symbol).

[alexander-shaposhnikov]

@jhenderson,
i don't know why binutils objcopy is doing it.

[[e-mail]2 ~/local] cat a.c
int xx = 12;
int yy() { return 11; }
[[e-mail]2 ~/local] gcc -c -g a.c -o a.o
[[e-mail]2 ~/local] objcopy --strip-all --keep-symbol yy a.o b.o
[[e-mail]2 ~/local] ~/llvm-readobj.sh -symbols b.o | grep Local

Binding: Local (0x0)
Binding: Local (0x0)
Binding: Local (0x0)
Binding: Local (0x0)
Binding: Local (0x0)
Binding: Local (0x0)
Binding: Local (0x0)

[[e-mail]2 ~/local] ~/llvm-readobj.sh -symbols b.o | grep yy

Name: yy (1)

[[e-mail]2 ~/local] ~/llvm-readobj.sh -symbols b.o | grep xx
[[e-mail]2 ~/local]

so basically we can implement this option without trying to repeat binutils behavior,
what would you say to this ? if so I can update the patch (and actually the code will be simpler)

[jhenderson]

In D47052#1106208, @alexshap wrote:

so basically we can implement this option without trying to repeat binutils behavior,
what would you say to this ? if so I can update the patch (and actually the code will be simpler)

I'm of the opinion that if the GNU behaviour doesn't make sense, we shouldn't follow it blindly, so I'd be happy in a divergence here, but I'm not a regular GNU tools user, so it might be wise to get @jakehehrlich's thoughts on this before proceeding.

[jakehehrlich]

In D47052#1106213, @jhenderson wrote:

In D47052#1106208, @alexshap wrote:

so basically we can implement this option without trying to repeat binutils behavior,
what would you say to this ? if so I can update the patch (and actually the code will be simpler)

I'm of the opinion that if the GNU behaviour doesn't make sense, we shouldn't follow it blindly, so I'd be happy in a divergence here, but I'm not a regular GNU tools user, so it might be wise to get @jakehehrlich's thoughts on this before proceeding.

I agree. This dosn't make sense to me. We should only keep the explicitly kept symbol and anything it relays on.

[alexander-shaposhnikov]

Update according to the previous discussion

[alexander-shaposhnikov]

add todo

[jakehehrlich]

LGTM but please wait on james

[alexander-shaposhnikov]

Fix typo in the comment

[jhenderson]

LGTM, with a few nits.

tools/llvm-objcopy/llvm-objcopy.cpp

227 ↗

(On Diff #147936)

Nit: effects -> affects

Also, I think you've wrapped unnecessarily. Clang-format doesn't "undo" line breaks in comments to match up to the column width.

381–384 ↗

(On Diff #147936)

See my earlier comments re. comment wrapping.

symbols table -> symbol table (x2)
strings table -> string table

386 ↗

(On Diff #147936)

I don't think you need Config in the capture list?

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

386 ↗

(On Diff #147936)

you are right, Config should not be captured here.

[alexander-shaposhnikov]

Fix typos.

llvm/trunk/test/tools/llvm-objcopy/strip-all-and-keep-symbol.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-all --keep-symbol foo %t %t2
+# RUN: llvm-readobj -sections -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
+  - Name:            .gnu.warning.foo
+    Type:            SHT_PROGBITS
+  - Name:            .debug_bar
+    Type:            SHT_PROGBITS
+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:     Name: .text
+# CHECK:     Name: .gnu.warning.foo
+# CHECK:     Name: .symtab
+# CHECK:     Name: .strtab
+# CHECK:     Name: .shstrtab
+# CHECK-NOT: Name: .debug_bar
+
+#CHECK: Symbols [
+#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:]

llvm/trunk/tools/llvm-objcopy/Object.h

protected:
 
   using SymPtr = std::unique_ptr<Symbol>;
 
 public:
   void addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                  SectionBase *DefinedIn, uint64_t Value, uint8_t Visibility,
                  uint16_t Shndx, uint64_t Sz);
   void addSymbolNames();
+  bool empty() const { return Symbols.empty(); }
   const SectionBase *getStrTab() const { return SymbolNames; }
   const Symbol *getSymbolByIndex(uint32_t Index) const;
   void updateSymbols(function_ref<void(Symbol &)> Callable);
 
   void removeSectionReferences(const SectionBase *Sec) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;

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

 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
 void HandleArgs(const CopyConfig &Config, Object &Obj, const Reader &Reader,
                 ElfType OutputElfType) {
 
   if (!Config.SplitDWO.empty()) {
     SplitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType);
   }
   
+  // TODO: update or remove symbols only if there is an option that affects them.
+  if (Obj.SymbolTable) {
+    Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
+      if ((Config.LocalizeHidden &&
+           (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
+          (!Config.SymbolsToLocalize.empty() &&
+           is_contained(Config.SymbolsToLocalize, Sym.Name)))
+        Sym.Binding = STB_LOCAL;
+
+      if (!Config.SymbolsToGlobalize.empty() &&
+          is_contained(Config.SymbolsToGlobalize, Sym.Name))
+        Sym.Binding = STB_GLOBAL;
+
+      if (!Config.SymbolsToWeaken.empty() &&
+          is_contained(Config.SymbolsToWeaken, Sym.Name) &&
+          Sym.Binding == STB_GLOBAL)
+        Sym.Binding = STB_WEAK;
+
+      if (Config.Weaken && Sym.Binding == STB_GLOBAL &&
+          Sym.getShndx() != SHN_UNDEF)
+        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.StripAll || Config.StripAllGNU)
+        return true;
+
+      if (!Config.SymbolsToRemove.empty() &&
+          is_contained(Config.SymbolsToRemove, Sym.Name)) {
+        return true;
+      }
+
+      return false;
+    });
+  }
+
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const SectionBase &Sec) {
       return std::find(std::begin(Config.ToRemove), std::end(Config.ToRemove),
                        Sec.Name) != std::end(Config.ToRemove);
     };
RemovePred = [Config, RemovePred](const SectionBase &Sec) {
       if (std::find(std::begin(Config.Keep), std::end(Config.Keep), Sec.Name) !=
           std::end(Config.Keep))
         return false;
       // Otherwise defer to RemovePred.
       return RemovePred(Sec);
     };
   }
 
+  // This has to be the last predicate assignment.
+  // If the option --keep-symbol has been specified
+  // and at least one of those symbols is present 
+  // (equivalently, the updated symbol table is not empty)
+  // the symbol table and the string table should not be removed.
+  if (!Config.SymbolsToKeep.empty() && !Obj.SymbolTable->empty()) {
+    RemovePred = [&Obj, RemovePred](const SectionBase &Sec) {
+      if (&Sec == Obj.SymbolTable || &Sec == Obj.SymbolTable->getStrTab())
+        return false;
+      return RemovePred(Sec);
+    };
+  }
+
   Obj.removeSections(RemovePred);
 
   if (!Config.AddSection.empty()) {
     for (const auto &Flag : Config.AddSection) {
       auto SecPair = Flag.split("=");
       auto SecName = SecPair.first;
       auto File = SecPair.second;
       auto BufOrErr = MemoryBuffer::getFile(File);
       if (!BufOrErr)
         reportError(File, BufOrErr.getError());
       auto Buf = std::move(*BufOrErr);
       auto BufPtr = reinterpret_cast<const uint8_t *>(Buf->getBufferStart());
       auto BufSize = Buf->getBufferSize();
       Obj.addSection<OwnedDataSection>(SecName,
                                        ArrayRef<uint8_t>(BufPtr, BufSize));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
-
-  if (Obj.SymbolTable) {
-    Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
-      if ((Config.LocalizeHidden &&
-           (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
-          (!Config.SymbolsToLocalize.empty() &&
-           is_contained(Config.SymbolsToLocalize, Sym.Name)))
-        Sym.Binding = STB_LOCAL;
-
-      if (!Config.SymbolsToGlobalize.empty() &&
-          is_contained(Config.SymbolsToGlobalize, Sym.Name))
-        Sym.Binding = STB_GLOBAL;
-
-      if (!Config.SymbolsToWeaken.empty() &&
-          is_contained(Config.SymbolsToWeaken, Sym.Name) &&
-          Sym.Binding == STB_GLOBAL)
-        Sym.Binding = STB_WEAK;
-
-      if (Config.Weaken && Sym.Binding == STB_GLOBAL &&
-          Sym.getShndx() != SHN_UNDEF)
-        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;
-      }
-
-      return false;
-    });
-  }
 }
 
 std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
                                      ElfType &OutputElfType) {
   // Right now we can only read ELF files so there's only one reader;
   auto Out = llvm::make_unique<ELFReader>(InputFilename);
   // We need to set the default ElfType for output.
   OutputElfType = Out->getElfType();