[llvm-objcopy] Add --weaken option

[paulsemel]

Summary

This option mark all global symbols as weaken.
This option follows the GNU objcopy --weaken option, as it doesn't mark the symbol as weak if this one is undefined (a relocation).

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

335–336

Can you make this two separate if statements?

[paulsemel]

Applied Jake's suggestions :

• Separated the two if statements

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

339

Why is this NameIndex check here?

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

339

This check tells me whether the symbol is defined or not. If not, it means that it's a relocation.
In this case (as GNU objdump does to), I'm not marking the symbol.

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

339

Sorry, I meant GNU objcopy..

[jakehehrlich]

tools/llvm-objcopy/llvm-objcopy.cpp

339

Well at the very least this isn't the right way to handle the situation you describe. I wasn't aware of this as it doesn't seem to be documented behavior (*highly* common with GNU objcopy). I'm going to ask around on what the behavior *should* be for the intended use cases to see what should happen. It sounds like we may want other 'weaken' options to do the same thing.

1. NameIndex is the index in the string table so it has nothing todo with weather a symbol is defined or not. It also isn't valid or meaningful until *after* finalization.
2. There isn't a good notion of "defined" in llvm-objcopy right now. There's 'defined in section X' but that dosn't cover absolute symbols and the like so there are some edge cases to check there.
3. Did we miss this behavior in the other 'weaken' options? It seems strange to me that this case would behave differently from the other weaken cases.

I've asked Roland McGrath whats supposed to be happening here but I don't suspect he'll get back to me until Monday. Sorry about that! I'll also do some black box testing of GNU objcopy to see what its doing but it isn't the case that we always want to copy it.

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

339

Well, you're definitely right ! I was looking for the section index, and I switched from getShndx() to NameIndex and I still can't explain why I did this..
But yes, it might neither respect the exact behavior of GNU objcopy, as you pointed out. I will wait for the answer of Roland McGrath then !
Anyway, about your third question, I am going to do some experiments to check whether I missed a behavior (which in my opinion is highly probable), and I'll come back to you !

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

339

Alright, the behavior is the same for the --weaken-symbol option. Anyway, my thoughts are that we might not need to change the behavior of this option.

Indeed, as the user explicitly want to weaken a symbol, I think we might weaken it in anyway; or at least, we might throw some sort of warnings (GNU objcopy silently does nothing..)

For this --weaken option, I would also go for this behavior (i.e. weaken every global symbols), as it is the behavior explained in the objcopy man page. Anyway, maybe we are missing something, and Roland McGrath will tell us why they made this choice of keeping undefined symbols global.

What do you think about it ?

[jhenderson]

tools/llvm-objcopy/llvm-objcopy.cpp

339

FWIW, checking getShndx() == SHN_UNDEF is the correct way to test for undefined symbols currently. This deals with absolute symbols etc happily.

On a related note, what is GNU objcopy's behaviour for --weaken etc and global absolute and common symbols?

[paulsemel]

Switching back from NameIndex to getShndx, as it is an any way the right thing to do here.
Waiting for Jake's feedback to fit better with GNU objcopy behavior.

[jakehehrlich]

So I asked the gods about this and got a half response on this issue but this appears to be intended for localize and globalize and *might* have been intended for weaken. After some thought I don't think it makes a lot of sense to weaken an undefined symbol and it might be unexpected behavior so I'm cool with it working this way. That would be my preference and it gives compatibility with objcopy.

Reading the code snippet Roland pointed out to me I don't think absolute symbols are treated specially but I don't know how common symbols are treated. Paul, can you do some blackbox testing of GNU objcopy to see how common symbols are treated?

tools/llvm-objcopy/llvm-objcopy.cpp

339

Ah! Good catch. I forgot about that.

[paulsemel]

Sure, here is what I've got so far :

Generally speaking, while copying, objcopy turns common symbols into object symbols. Otherway, it goes the same as it is with other symbol types.

For --globalize-symbol option :

• doesn't do anything for weak symbols
• works with undefined local symbols

For --localize-symbol option:

• works with all (but undef of course) symbols

So, do you think that I need to change the behavior of those two options to match the objcopy behavior ?

Thanks for taking the time to ask Roland McGrath !

[jakehehrlich]

I'm happy with this change. I think we should change --globalize and --localize to match but if --globalize-symbol or --localize-symbol is used it shouldn't bother checking; if people want to do non-sense that's their choice. I didn't think about it but it makes a lot of sense not to globalize weak symbols and it makes sense to not localize undefined symbols. So I think the choices objcopy chose makes sense. If you could change those to match that, it would be awesome!

[paulsemel]

In D46217#1084850, @jakehehrlich wrote:

I'm happy with this change. I think we should change --globalize and --localize to match but if --globalize-symbol or --localize-symbol is used it shouldn't bother checking; if people want to do non-sense that's their choice. I didn't think about it but it makes a lot of sense not to globalize weak symbols and it makes sense to not localize undefined symbols. So I think the choices objcopy chose makes sense. If you could change those to match that, it would be awesome!

Cool, nice to hear ! However, I don't think there is --globalize and --localize option implemented.. so I don't really see what you want me to change

[jakehehrlich]

In D46217#1085019, @paulsemel wrote:

In D46217#1084850, @jakehehrlich wrote:

I'm happy with this change. I think we should change --globalize and --localize to match but if --globalize-symbol or --localize-symbol is used it shouldn't bother checking; if people want to do non-sense that's their choice. I didn't think about it but it makes a lot of sense not to globalize weak symbols and it makes sense to not localize undefined symbols. So I think the choices objcopy chose makes sense. If you could change those to match that, it would be awesome!

Cool, nice to hear ! However, I don't think there is --globalize and --localize option implemented.. so I don't really see what you want me to change

I have an imaginative memory apparently. For some reason I thought you implemented those. Whoops. LGTM on this

[paulsemel]

commited on r331397

test/tools/llvm-objcopy/weaken-all.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --weaken %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
+  - Name:            .rel.text
+    Type:            SHT_REL
+    Info:            .text
+    Relocations:
+      - Offset: 0x1000
+        Symbol: undefGlobal
+        Type:   R_X86_64_PC32
+Symbols:
+  Local:
+    - Name:     Local
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1008
+      Size:     8
+  Weak:
+    - Name:     Weak
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1010
+  Global:
+    - Name:     Global
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1018
+    - Name:     undefGlobal
+      Type:     STT_FUNC
+      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: Local
+#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:  Symbol {
+#CHECK-NEXT:    Name: Global
+#CHECK-NEXT:    Value: 0x1018
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Weak
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: undefGlobal
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Global
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section:
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: Weak
+#CHECK-NEXT:    Value: 0x1010
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Weak
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/Opts.td

 defm globalize_symbol : Eq<"globalize-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Mark <symbol> as global">;
 defm weaken_symbol : Eq<"weaken-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Mark <symbol> as weak">;
 def W : JoinedOrSeparate<["-"], "W">,
         Alias<weaken_symbol>;
+def weaken : Flag<["-", "--"], "weaken">,
+                  HelpText<"Mark all global symbols as weak">;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   bool StripAll;
   bool StripAllGNU;
   bool StripDebug;
   bool StripSections;
   bool StripNonAlloc;
   bool StripDWO;
   bool ExtractDWO;
   bool LocalizeHidden;
+  bool Weaken;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 bool IsDWOSection(const SectionBase &Sec) { return Sec.Name.endswith(".dwo"); }
 
 bool OnlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
         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();
     });
   }
 }
 
 std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
+  Config.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   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());
 
   return Config;