[PATCH] D65893: [llvm-objcopy] Allow the visibility of symbols created by --binary and --add-symbol to be specified with --new-symbol-visibility

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the update. It looks better now. Will accept after a few comments are addressed.



================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:494
 
+  if (InputArgs.hasArg(OBJCOPY_new_symbol_visibility)) {
+    const uint8_t Invalid = 0xff;
----------------
```
  if (opt::Arg *A = InputArgs.getLastArg(OBJCOPY_new_symbol_visibility)) {
    ... StringSwitch<uint8_t>(A->getValue())
```


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:720
   for (auto Arg : InputArgs.filtered(OBJCOPY_add_symbol)) {
-    Expected<NewSymbolInfo> NSI = parseNewSymbolInfo(Arg->getValue());
+    uint8_t DefaultVisibility = Config.NewSymbolVisibility
+                                ? *Config.NewSymbolVisibility
----------------
`getValueOr(ELF::STV_DEFAULT)`


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:759
+  uint8_t NewSymbolVisibility =
+      (Config.NewSymbolVisibility) ? *Config.NewSymbolVisibility : STV_DEFAULT;
+  BinaryReader Reader(Config.BinaryArch, NewSymbolVisibility, &In);
----------------
`getValueOr(ELF::STV_DEFAULT)`


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:897
+  BinaryELFBuilder(uint16_t EM, uint8_t NewSymbolVisibility, MemoryBuffer *MB)
+      : BasicELFBuilder(EM), NewSymbolVisibility(NewSymbolVisibility),
+        MemBuf(MB) {}
----------------
Move NewSymbolVisibility after MemBuf [-Wreorder]


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:952
+               MemoryBuffer *MB)
+      : MInfo(MI), NewSymbolVisibility(NewSymbolVisibility), MemBuf(MB) {}
   std::unique_ptr<Object> create() const override;
----------------
Move NewSymbolVisibility after MemBuf


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65893/new/

https://reviews.llvm.org/D65893