[llvm-objcopy][NFC] Refactor CopyConfig structure - remove lazy options processing.

[avl]

Summary

During reviewing D102277 it was decided to remove lazy options processing
from llvm-objcopy CopyConfig structure. This patch transforms processing of ELF
lazy options into the in-place processing.

[avl]

added minor refactoring.

[avl]

changed String flag values to the enum.

[jhenderson]

LGTM overall. Just a few nits.

FYI, I'm off work all of next week, so it'll be the week after before I can comment on any more patches.

llvm/tools/llvm-objcopy/CommonConfig.h

155

It's not explicitly stated, but reading between the lines, and looking at the example in the coding standards, I think this should be IndirectFunction (and UniqueObject below).

165

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

503–504

I think this keeps the comment a little more generic, so that if the signature changes, the comment remains valid.

[avl]

LGTM overall. Just a few nits.

FYI, I'm off work all of next week, so it'll be the week after before I can comment on any more patches.

I see, thanks. And thank you for reviewing.

[avl]

addressed comments. rebased to have a buildbot report.

llvm/tools/llvm-objcopy/CommonConfig.h

Error addMatcher(Expected<NameOrPattern> Matcher) {
     return Error::success();
   }
   bool matches(StringRef S) const {
     return is_contained(PosMatchers, S) && !is_contained(NegMatchers, S);
   }
   bool empty() const { return PosMatchers.empty() && NegMatchers.empty(); }
 };
 
+enum class SymbolFlag {
+  Global,
+  Local,
+  Weak,
+  Default,
+  Hidden,
+  Protected,
+  File,
+  Section,
+  Object,
+  Function,
+  IndirectFunction,
+  Debug,
+  Constructor,
+  Warning,
+  Indirect,
+  Synthetic,
+  UniqueObject,
+};
+
+// Symbol info specified by --add-symbol option. Symbol flags not supported
+// by a concrete format should be ignored.
+struct NewSymbolInfo {
+  StringRef SymbolName;
+  StringRef SectionName;
+  uint64_t Value = 0;
+  std::vector<SymbolFlag> Flags;
+  std::vector<StringRef> BeforeSyms;
+};
+
 // Configuration for copying/stripping a single file.
 struct CommonConfig {
   // Main input/output options
   StringRef InputFilename;
   FileFormat InputFormat = FileFormat::Unspecified;
   StringRef OutputFilename;
   FileFormat OutputFormat = FileFormat::Unspecified;
 
struct CommonConfig {
   StringMap<StringRef> SymbolsToRename;
 
   // ELF entry point address expression. The input parameter is an entry point
   // address in the input ELF file. The entry address in the output file is
   // calculated with EntryExpr(input_address), when either --set-start or
   // --change-start is used.
   std::function<uint64_t(uint64_t)> EntryExpr;
 
+  // Symbol info specified by --add-symbol option.
+  std::vector<NewSymbolInfo> SymbolsToAdd;
+
   // Boolean options
   bool AllowBrokenLinks = false;
   bool DeterministicArchives = true;
   bool ExtractDWO = false;
   bool ExtractMainPartition = false;
   bool KeepFileSymbols = false;
   bool KeepUndefined = false;
   bool LocalizeHidden = false;

llvm/tools/llvm-objcopy/ConfigManager.h

struct ConfigManager : public MultiFormatConfig {
   virtual ~ConfigManager() {}
 
   const CommonConfig &getCommonConfig() const override { return Common; }
   Expected<const ELFConfig &> getELFConfig() const override;
   Expected<const COFFConfig &> getCOFFConfig() const override;
   Expected<const MachOConfig &> getMachOConfig() const override;
   Expected<const WasmConfig &> getWasmConfig() const override;
 
-  // String representation for lazy ELF options.
-  std::vector<StringRef> SymbolsToAdd;
-  Optional<StringRef> NewSymbolVisibility;
-
   // All configs.
   CommonConfig Common;
-  mutable Optional<ELFConfig> ELF;
+  ELFConfig ELF;
   COFFConfig COFF;
   MachOConfig MachO;
   WasmConfig Wasm;
 };
 
 // Configuration for the overall invocation of this tool. When invoked as
 // objcopy, will always contain exactly one CopyConfig. When invoked as strip,
 // will contain one or more CopyConfigs.

llvm/tools/llvm-objcopy/ConfigManager.cpp

static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
   OptTable.PrintHelp(OS, (ToolName + HelpText).str().c_str(),
                      (ToolName + " tool").str().c_str());
   // TODO: Replace this with libOption call once it adds extrahelp support.
   // The CommandLine library has a cl::extrahelp class to support this,
   // but libOption does not have that yet.
   OS << "\nPass @FILE as argument to read options from FILE.\n";
 }
 
-static Expected<NewSymbolInfo> parseNewSymbolInfo(StringRef FlagValue,
-                                                  uint8_t DefaultVisibility) {
+static Expected<NewSymbolInfo> parseNewSymbolInfo(StringRef FlagValue) {
   // Parse value given with --add-symbol option and create the
   // new symbol if possible. The value format for --add-symbol is:
   //
   // <name>=[<section>:]<value>[,<flags>]
   //
   // where:
   // <name> - symbol name, can be empty string
   // <section> - optional section name. If not given ABS symbol is created
   // <value> - symbol value, can be decimal or hexadecimal number prefixed
   //           with 0x.
-  // <flags> - optional flags affecting symbol type, binding or visibility:
-  //           The following are currently supported:
-  //
-  //           global, local, weak, default, hidden, file, section, object,
-  //           indirect-function.
-  //
-  //           The following flags are ignored and provided for GNU
-  //           compatibility only:
-  //
-  //           warning, debug, constructor, indirect, synthetic,
-  //           unique-object, before=<symbol>.
+  // <flags> - optional flags affecting symbol type, binding or visibility.
   NewSymbolInfo SI;
   StringRef Value;
   std::tie(SI.SymbolName, Value) = FlagValue.split('=');
   if (Value.empty())
     return createStringError(
         errc::invalid_argument,
         "bad format for --add-symbol, missing '=' after '%s'",
         SI.SymbolName.str().c_str());
 
   if (Value.contains(':')) {
     std::tie(SI.SectionName, Value) = Value.split(':');
     if (SI.SectionName.empty() || Value.empty())
       return createStringError(
           errc::invalid_argument,
           "bad format for --add-symbol, missing section name or symbol value");
   }
 
   SmallVector<StringRef, 6> Flags;
   Value.split(Flags, ',');
   if (Flags[0].getAsInteger(0, SI.Value))
     return createStringError(errc::invalid_argument, "bad symbol value: '%s'",
                              Flags[0].str().c_str());
 
-  SI.Visibility = DefaultVisibility;
-  using Functor = std::function<void(void)>;
+  using Functor = std::function<void()>;
   SmallVector<StringRef, 6> UnsupportedFlags;
   for (size_t I = 1, NumFlags = Flags.size(); I < NumFlags; ++I)
     static_cast<Functor>(
         StringSwitch<Functor>(Flags[I])
-            .CaseLower("global", [&SI] { SI.Bind = ELF::STB_GLOBAL; })
-            .CaseLower("local", [&SI] { SI.Bind = ELF::STB_LOCAL; })
-            .CaseLower("weak", [&SI] { SI.Bind = ELF::STB_WEAK; })
-            .CaseLower("default", [&SI] { SI.Visibility = ELF::STV_DEFAULT; })
-            .CaseLower("hidden", [&SI] { SI.Visibility = ELF::STV_HIDDEN; })
+            .CaseLower("global",
+                       [&] { SI.Flags.push_back(SymbolFlag::Global); })
+            .CaseLower("local", [&] { SI.Flags.push_back(SymbolFlag::Local); })
+            .CaseLower("weak", [&] { SI.Flags.push_back(SymbolFlag::Weak); })
+            .CaseLower("default",
+                       [&] { SI.Flags.push_back(SymbolFlag::Default); })
+            .CaseLower("hidden",
+                       [&] { SI.Flags.push_back(SymbolFlag::Hidden); })
             .CaseLower("protected",
-                       [&SI] { SI.Visibility = ELF::STV_PROTECTED; })
-            .CaseLower("file", [&SI] { SI.Type = ELF::STT_FILE; })
-            .CaseLower("section", [&SI] { SI.Type = ELF::STT_SECTION; })
-            .CaseLower("object", [&SI] { SI.Type = ELF::STT_OBJECT; })
-            .CaseLower("function", [&SI] { SI.Type = ELF::STT_FUNC; })
-            .CaseLower("indirect-function",
-                       [&SI] { SI.Type = ELF::STT_GNU_IFUNC; })
-            .CaseLower("debug", [] {})
-            .CaseLower("constructor", [] {})
-            .CaseLower("warning", [] {})
-            .CaseLower("indirect", [] {})
-            .CaseLower("synthetic", [] {})
-            .CaseLower("unique-object", [] {})
-            .StartsWithLower("before", [] {})
+                       [&] { SI.Flags.push_back(SymbolFlag::Protected); })
+            .CaseLower("file", [&] { SI.Flags.push_back(SymbolFlag::File); })
+            .CaseLower("section",
+                       [&] { SI.Flags.push_back(SymbolFlag::Section); })
+            .CaseLower("object",
+                       [&] { SI.Flags.push_back(SymbolFlag::Object); })
+            .CaseLower("function",
+                       [&] { SI.Flags.push_back(SymbolFlag::Function); })
+            .CaseLower(
+                "indirect-function",
+                [&] { SI.Flags.push_back(SymbolFlag::IndirectFunction); })
+            .CaseLower("debug", [&] { SI.Flags.push_back(SymbolFlag::Debug); })
+            .CaseLower("constructor",
+                       [&] { SI.Flags.push_back(SymbolFlag::Constructor); })
+            .CaseLower("warning",
+                       [&] { SI.Flags.push_back(SymbolFlag::Warning); })
+            .CaseLower("indirect",
+                       [&] { SI.Flags.push_back(SymbolFlag::Indirect); })
+            .CaseLower("synthetic",
+                       [&] { SI.Flags.push_back(SymbolFlag::Synthetic); })
+            .CaseLower("unique-object",
+                       [&] { SI.Flags.push_back(SymbolFlag::UniqueObject); })
+            .StartsWithLower("before=",
+                             [&] {
+                               StringRef SymNamePart =
+                                   Flags[I].split('=').second;
+
+                               if (!SymNamePart.empty())
+                                 SI.BeforeSyms.push_back(SymNamePart);
+                             })
             .Default([&] { UnsupportedFlags.push_back(Flags[I]); }))();
   if (!UnsupportedFlags.empty())
     return createStringError(errc::invalid_argument,
                              "unsupported flag%s for --add-symbol: '%s'",
                              UnsupportedFlags.size() > 1 ? "s" : "",
                              join(UnsupportedFlags, "', '").c_str());
+
   return SI;
 }
 
 Expected<const ELFConfig &> ConfigManager::getELFConfig() const {
-  if (!ELF) {
-    if (Common.StripSwiftSymbols || Common.KeepUndefined)
-      return createStringError(llvm::errc::invalid_argument,
-                               "option not supported by llvm-objcopy for ELF");
 
-    // Parse lazy options.
-    ELFConfig ResConfig;
-
-    if (NewSymbolVisibility) {
-      const uint8_t Invalid = 0xff;
-      ResConfig.NewSymbolVisibility =
-          StringSwitch<uint8_t>(*NewSymbolVisibility)
-              .Case("default", ELF::STV_DEFAULT)
-              .Case("hidden", ELF::STV_HIDDEN)
-              .Case("internal", ELF::STV_INTERNAL)
-              .Case("protected", ELF::STV_PROTECTED)
-              .Default(Invalid);
-
-      if (ResConfig.NewSymbolVisibility == Invalid)
-        return createStringError(errc::invalid_argument,
-                                 "'%s' is not a valid symbol visibility",
-                                 NewSymbolVisibility->str().c_str());
-    }
-
-    for (StringRef Arg : SymbolsToAdd) {
-      Expected<NewSymbolInfo> NSI = parseNewSymbolInfo(
-          Arg,
-          ResConfig.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT));
-      if (!NSI)
-        return NSI.takeError();
-      ResConfig.SymbolsToAdd.push_back(*NSI);
-    }
-
-    ELF = std::move(ResConfig);
-  }
-
-  return *ELF;
+  return ELF;
 }
 
 Expected<const COFFConfig &> ConfigManager::getCOFFConfig() const {
   if (Common.AllowBrokenLinks || !Common.SplitDWO.empty() ||
       !Common.SymbolsPrefix.empty() || !Common.AllocSectionsPrefix.empty() ||
       !Common.DumpSection.empty() || !Common.KeepSection.empty() ||
-      NewSymbolVisibility || !Common.SymbolsToGlobalize.empty() ||
+      ELF.NewSymbolVisibility || !Common.SymbolsToGlobalize.empty() ||
       !Common.SymbolsToKeep.empty() || !Common.SymbolsToLocalize.empty() ||
       !Common.SymbolsToWeaken.empty() || !Common.SymbolsToKeepGlobal.empty() ||
       !Common.SectionsToRename.empty() || !Common.SetSectionAlignment.empty() ||
       Common.ExtractDWO || Common.LocalizeHidden || Common.PreserveDates ||
       Common.StripDWO || Common.StripNonAlloc || Common.StripSections ||
       Common.StripSwiftSymbols || Common.KeepUndefined || Common.Weaken ||
       Common.DecompressDebugSections ||
-      Common.DiscardMode == DiscardType::Locals || !SymbolsToAdd.empty() ||
-      Common.EntryExpr) {
+      Common.DiscardMode == DiscardType::Locals ||
+      !Common.SymbolsToAdd.empty() || Common.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for COFF");
   }
 
   return COFF;
 }
 
 Expected<const MachOConfig &> ConfigManager::getMachOConfig() const {
   if (Common.AllowBrokenLinks || !Common.SplitDWO.empty() ||
       !Common.SymbolsPrefix.empty() || !Common.AllocSectionsPrefix.empty() ||
-      !Common.KeepSection.empty() || NewSymbolVisibility ||
+      !Common.KeepSection.empty() || ELF.NewSymbolVisibility ||
       !Common.SymbolsToGlobalize.empty() || !Common.SymbolsToKeep.empty() ||
       !Common.SymbolsToLocalize.empty() || !Common.SymbolsToWeaken.empty() ||
       !Common.SymbolsToKeepGlobal.empty() || !Common.SectionsToRename.empty() ||
       !Common.UnneededSymbolsToRemove.empty() ||
       !Common.SetSectionAlignment.empty() || !Common.SetSectionFlags.empty() ||
       Common.ExtractDWO || Common.LocalizeHidden || Common.PreserveDates ||
       Common.StripAllGNU || Common.StripDWO || Common.StripNonAlloc ||
       Common.StripSections || Common.Weaken || Common.DecompressDebugSections ||
       Common.StripUnneeded || Common.DiscardMode == DiscardType::Locals ||
-      !SymbolsToAdd.empty() || Common.EntryExpr) {
+      !Common.SymbolsToAdd.empty() || Common.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
   }
 
   return MachO;
 }
 
 Expected<const WasmConfig &> ConfigManager::getWasmConfig() const {
   if (!Common.AddGnuDebugLink.empty() || Common.ExtractPartition ||
       !Common.SplitDWO.empty() || !Common.SymbolsPrefix.empty() ||
       !Common.AllocSectionsPrefix.empty() ||
-      Common.DiscardMode != DiscardType::None || NewSymbolVisibility ||
-      !SymbolsToAdd.empty() || !Common.RPathToAdd.empty() ||
+      Common.DiscardMode != DiscardType::None || ELF.NewSymbolVisibility ||
+      !Common.SymbolsToAdd.empty() || !Common.RPathToAdd.empty() ||
       !Common.OnlySection.empty() || !Common.SymbolsToGlobalize.empty() ||
       !Common.SymbolsToKeep.empty() || !Common.SymbolsToLocalize.empty() ||
       !Common.SymbolsToRemove.empty() ||
       !Common.UnneededSymbolsToRemove.empty() ||
       !Common.SymbolsToWeaken.empty() || !Common.SymbolsToKeepGlobal.empty() ||
       !Common.SectionsToRename.empty() || !Common.SetSectionAlignment.empty() ||
       !Common.SetSectionFlags.empty() || !Common.SymbolsToRename.empty()) {
     return createStringError(
if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 2)
     return createStringError(errc::invalid_argument,
                              "too many positional arguments");
 
   ConfigManager ConfigMgr;
   CommonConfig &Config = ConfigMgr.Common;
+  ELFConfig &ELFConfig = ConfigMgr.ELF;
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
   if (InputArgs.hasArg(OBJCOPY_target) &&
       (InputArgs.hasArg(OBJCOPY_input_target) ||
        InputArgs.hasArg(OBJCOPY_output_target)))
     return createStringError(
         errc::invalid_argument,
         "--target cannot be used with --input-target or --output-target");
parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
   // FIXME:  Currently, we ignore the target for non-binary/ihex formats
   // explicitly specified by -I option (e.g. -Ielf32-x86-64) and guess the
   // format by llvm::object::createBinary regardless of the option value.
   Config.InputFormat = StringSwitch<FileFormat>(InputFormat)
                            .Case("binary", FileFormat::Binary)
                            .Case("ihex", FileFormat::IHex)
                            .Default(FileFormat::Unspecified);
 
-  if (InputArgs.hasArg(OBJCOPY_new_symbol_visibility))
-    ConfigMgr.NewSymbolVisibility =
+  if (InputArgs.hasArg(OBJCOPY_new_symbol_visibility)) {
+    const uint8_t Invalid = 0xff;
+    StringRef VisibilityStr =
         InputArgs.getLastArgValue(OBJCOPY_new_symbol_visibility);
 
+    ELFConfig.NewSymbolVisibility = StringSwitch<uint8_t>(VisibilityStr)
+                                        .Case("default", ELF::STV_DEFAULT)
+                                        .Case("hidden", ELF::STV_HIDDEN)
+                                        .Case("internal", ELF::STV_INTERNAL)
+                                        .Case("protected", ELF::STV_PROTECTED)
+                                        .Default(Invalid);
+
+    if (ELFConfig.NewSymbolVisibility == Invalid)
+      return createStringError(errc::invalid_argument,
+                               "'%s' is not a valid symbol visibility",
+                               VisibilityStr.str().c_str());
+  }
+
   Config.OutputFormat = StringSwitch<FileFormat>(OutputFormat)
                             .Case("binary", FileFormat::Binary)
                             .Case("ihex", FileFormat::IHex)
                             .Default(FileFormat::Unspecified);
   if (Config.OutputFormat == FileFormat::Unspecified) {
     if (OutputFormat.empty()) {
       Config.OutputFormat = Config.InputFormat;
     } else {
for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     if (Error E = Config.SymbolsToKeep.addMatcher(NameOrPattern::create(
             Arg->getValue(), SymbolMatchStyle, ErrorCallback)))
       return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbols))
     if (Error E =
             addSymbolsFromFile(Config.SymbolsToKeep, DC.Alloc, Arg->getValue(),
                                SymbolMatchStyle, ErrorCallback))
       return std::move(E);
-  for (auto Arg : InputArgs.filtered(OBJCOPY_add_symbol))
-    ConfigMgr.SymbolsToAdd.push_back(Arg->getValue());
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_add_symbol)) {
+    Expected<NewSymbolInfo> SymInfo = parseNewSymbolInfo(Arg->getValue());
+    if (!SymInfo)
+      return SymInfo.takeError();
+
+    Config.SymbolsToAdd.push_back(*SymInfo);
+  }
 
   Config.AllowBrokenLinks = InputArgs.hasArg(OBJCOPY_allow_broken_links);
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);

llvm/tools/llvm-objcopy/ELF/ELFConfig.h

 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELFTypes.h"
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 
-struct NewSymbolInfo {
-  StringRef SymbolName;
-  StringRef SectionName;
-  uint64_t Value = 0;
-  uint8_t Type = ELF::STT_NOTYPE;
-  uint8_t Bind = ELF::STB_GLOBAL;
-  uint8_t Visibility = ELF::STV_DEFAULT;
-};
-
 // ELF specific configuration for copying/stripping a single file.
 struct ELFConfig {
-  Optional<uint8_t> NewSymbolVisibility;
-  std::vector<NewSymbolInfo> SymbolsToAdd;
+  uint8_t NewSymbolVisibility = (uint8_t)ELF::STV_DEFAULT;
 };
 
 } // namespace objcopy
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_OBJCOPY_ELF_ELFCONFIG_H

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

if (Error Err = replaceDebugSections(
               return &Obj.addSection<DecompressedSection>(*CS);
             }))
       return Err;
   }
 
   return Obj.removeSections(Config.AllowBrokenLinks, RemovePred);
 }
 
+// Add symbol to the Object symbol table with the specified properties.
+static void addSymbol(Object &Obj, const NewSymbolInfo &SymInfo,
+                      uint8_t DefaultVisibility) {
+  SectionBase *Sec = Obj.findSection(SymInfo.SectionName);
+  uint64_t Value = Sec ? Sec->Addr + SymInfo.Value : SymInfo.Value;
+
+  uint8_t Bind = ELF::STB_GLOBAL;
+  uint8_t Type = ELF::STT_NOTYPE;
+  uint8_t Visibility = DefaultVisibility;
+
+  for (SymbolFlag FlagValue : SymInfo.Flags)
+    switch (FlagValue) {
+    case SymbolFlag::Global:
+      Bind = ELF::STB_GLOBAL;
+      break;
+    case SymbolFlag::Local:
+      Bind = ELF::STB_LOCAL;
+      break;
+    case SymbolFlag::Weak:
+      Bind = ELF::STB_WEAK;
+      break;
+    case SymbolFlag::Default:
+      Visibility = ELF::STV_DEFAULT;
+      break;
+    case SymbolFlag::Hidden:
+      Visibility = ELF::STV_HIDDEN;
+      break;
+    case SymbolFlag::Protected:
+      Visibility = ELF::STV_PROTECTED;
+      break;
+    case SymbolFlag::File:
+      Type = ELF::STT_FILE;
+      break;
+    case SymbolFlag::Section:
+      Type = ELF::STT_SECTION;
+      break;
+    case SymbolFlag::Object:
+      Type = ELF::STT_OBJECT;
+      break;
+    case SymbolFlag::Function:
+      Type = ELF::STT_FUNC;
+      break;
+    case SymbolFlag::IndirectFunction:
+      Type = ELF::STT_GNU_IFUNC;
+      break;
+    default: /* Other flag values are ignored for ELF. */
+      break;
+    };
+
+  Obj.SymbolTable->addSymbol(
+      SymInfo.SymbolName, Bind, Type, Sec, Value, Visibility,
+      Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
+}
+
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // 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.
 static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
   // If the symbol table was previously removed, we need to create a new one
   // before adding new symbols.
-  if (!Obj.SymbolTable && !ELFConfig.SymbolsToAdd.empty())
+  if (!Obj.SymbolTable && !Config.SymbolsToAdd.empty())
     if (Error E = Obj.addNewSymbolTable())
       return E;
 
-  for (const NewSymbolInfo &SI : ELFConfig.SymbolsToAdd) {
-    SectionBase *Sec = Obj.findSection(SI.SectionName);
-    uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
-    Obj.SymbolTable->addSymbol(
-        SI.SymbolName, SI.Bind, SI.Type, Sec, Value, SI.Visibility,
-        Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
-  }
+  for (const NewSymbolInfo &SI : Config.SymbolsToAdd)
+    addSymbol(Obj, SI, ELFConfig.NewSymbolVisibility);
 
   // --set-section-flags works with sections added by --add-section.
   if (!Config.SetSectionFlags.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SetSectionFlags.find(Sec.Name);
       if (Iter != Config.SetSectionFlags.end()) {
         const SectionFlagsUpdate &SFU = Iter->second;
         setSectionFlagsAndType(Sec, SFU.NewFlags);
Error executeObjcopyOnIHex(const CommonConfig &Config,
   if (Error E = handleArgs(Config, ELFConfig, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
 Error executeObjcopyOnRawBinary(const CommonConfig &Config,
                                 const ELFConfig &ELFConfig, MemoryBuffer &In,
                                 raw_ostream &Out) {
-  uint8_t NewSymbolVisibility =
-      ELFConfig.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
-  BinaryReader Reader(&In, NewSymbolVisibility);
+  BinaryReader Reader(&In, ELFConfig.NewSymbolVisibility);
   Expected<std::unique_ptr<Object>> Obj = Reader.create(true);
   if (!Obj)
     return Obj.takeError();
 
   // Prefer OutputArch (-O<format>) if set, otherwise fallback to BinaryArch
   // (-B<arch>).
   const ElfType OutputElfType =
       getOutputElfType(Config.OutputArch.getValueOr(MachineInfo()));
   if (Error E = handleArgs(Config, ELFConfig, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
 Error executeObjcopyOnBinary(const CommonConfig &Config,
                              const ELFConfig &ELFConfig,
                              object::ELFObjectFileBase &In, raw_ostream &Out) {
   ELFReader Reader(&In, Config.ExtractPartition);
   Expected<std::unique_ptr<Object>> Obj =
-      Reader.create(!ELFConfig.SymbolsToAdd.empty());
+      Reader.create(!Config.SymbolsToAdd.empty());
   if (!Obj)
     return Obj.takeError();
   // Prefer OutputArch (-O<format>) if set, otherwise infer it from the input.
   const ElfType OutputElfType =
       Config.OutputArch ? getOutputElfType(Config.OutputArch.getValue())
                         : getOutputElfType(In);
 
   if (Error E = handleArgs(Config, ELFConfig, **Obj))