[llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC.

[seiya]

Summary

This patch splits the command-line parsing into two phases:

First, parse cross-platform options and leave ELF-specific options unparsed.

Second, in the ELF implementation, parse ELF-specific options and construct ELFCopyConfig.

[alexander-shaposhnikov]

just want to mention a slightly different approach:

CopyConfig.h:

// Forward declarations only.
struct COFFCopyConfig;
struct ELFCopyConfig;
struct MachOCopyConfig;

struct CopyConfig {
    ...
   Optional<StringRef> NewSymbolVisibility;
   std::vector<StringRef> SymbolsToAdd;
  
   std::unique_ptr<COFFCopyConfig> COFF;
   std::unique_ptr<ELFCopyConfig> ELF;
   std::unique_ptr<MachOCopyConfig> MachO;
};

ELFObjcopy.h:

struct ELFCopyConfig {
   Optional<uint8_t> NewSymbolVisibility;
   std::vector<NewSymbolInfo> SymbolsToAdd
};

Error parseELFCopyConfig(CopyConfig &C);

Error executeObjcopyOnBinary(
     CopyConfig &C,
     object::ELFObjectFileBase &In, Buffer &Out);

What do you think ?

[jhenderson]

I've got another suggestion: have separate COFFConfig/ELFConfig etc classes that only contain the interpreted versions and lazily construct them prior to calling the corresponding code path. CopyConfig would contain the raw unparsed strings (perhaps rename them to "RawSymbolsToAdd" etc). Perhaps a higher-level Config class could exist just to wrap them and pass them to executeObjcopyOnBinary etc.

Rough outline:

CopyConfig.h:

class CopyConfig {
  std::vector<StringRef> RawSymbolsToAdd;
  ...
};

class ELFConfig {
  std::vector<NewSymbolInfo> SymbolsToAdd;
  ...
};

// etc for COFF, Mach-O...

llvm-objcopy.cpp:

struct Configurations {
  CopyConfig Common;
  Optional<ELFConfig> ELF;
  ...
}

...

static Error executeObjcopyOnBinary(Configurations &Cfgs, ...) {
  if (/*isElf*/...) {
    if (!Cfgs.ELF)
     Cfgs.ELF.emplace(Cfgs.Common);
    return elf::executeObjcopyOnBinary(Cfgs.Common, Cfgs.ELF, ...)
  }
  ...
}

On a related note, I think the ELF-specific etc parsing code belongs in a file other than ELFObjcopy.cpp. I feel like that file should be confined to doing the actual objcopy processing, whereas the command-line option parsing etc should be separate, e.g. in a new file called ELFConfig.cpp etc. What do you think?

[alexander-shaposhnikov]

P.S. i'm ok with @jhenderson's suggestion.

[seiya]

In D67139#1657802, @jhenderson wrote:

I've got another suggestion: have separate COFFConfig/ELFConfig etc classes that only contain the interpreted versions and lazily construct them prior to calling the corresponding code path. CopyConfig would contain the raw unparsed strings (perhaps rename them to "RawSymbolsToAdd" etc). Perhaps a higher-level Config class could exist just to wrap them and pass them to executeObjcopyOnBinary etc.

Rough outline:

CopyConfig.h:

class CopyConfig {
  std::vector<StringRef> RawSymbolsToAdd;
  ...
};

class ELFConfig {
  std::vector<NewSymbolInfo> SymbolsToAdd;
  ...
};

// etc for COFF, Mach-O...

llvm-objcopy.cpp:

struct Configurations {
  CopyConfig Common;
  Optional<ELFConfig> ELF;
  ...
}

...

static Error executeObjcopyOnBinary(Configurations &Cfgs, ...) {
  if (/*isElf*/...) {
    if (!Cfgs.ELF)
     Cfgs.ELF.emplace(Cfgs.Common);
    return elf::executeObjcopyOnBinary(Cfgs.Common, Cfgs.ELF, ...)
  }
  ...
}

LGTM. I'll implement the idea.

On a related note, I think the ELF-specific etc parsing code belongs in a file other than ELFObjcopy.cpp. I feel like that file should be confined to doing the actual objcopy processing, whereas the command-line option parsing etc should be separate, e.g. in a new file called ELFConfig.cpp etc. What do you think?

I agree with this. I'll add ELFConfig.cpp to separate the command-line parsing.

[seiya]

Adopt @jhenderson's idea D67139#1657802.

[jhenderson]

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

135 ↗

(On Diff #219870)

For some warning levels, this will produce a warning as it is unused. Probably, you should comment out the name: const ELFCopyConfig &/*ELFConfig*/.

Of course, this becomes moot if you follow my suggestion of only passing in ELFConfig.

172–173 ↗

(On Diff #219870)

This and several other functions don't look like they need to change their signatures, since you don't use the ELF-specific parameters. Please revert them, or switch to passing in only ELFConfig and NOT Config.

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

29–30 ↗

(On Diff #219870)

Since ELFConfig has Config as a member, you could just pass in ELFConfig in this and the other functions here, and use that.

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

271 ↗

(On Diff #219870)

Here and elsewhere, I'd keep the name as Config.

[seiya]

Pass only ELFCopyConfig instead of passing both CopyConfig and ELFCopyConfig.

[jhenderson]

LGTM. Might want to give a chance for somebody else to approve too.

[alexander-shaposhnikov]

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

93 ↗

(On Diff #220054)

frankly speaking I'm not a big fan of this design again, i kinda read James' suggestion differently.
Here we have this duplicated reference structure (one reference to Common here, another one is inside ELFConfig) and the responsibilities / concerns of these classes are kinda intricated.

If we want to separate Common from everything else - I'm totally fine with that, but would prefer to have a simpler structure here:

struct DriverConfig {
    SmallVector<Configuration, 1> Configurations; 
    BumpPtrAllocator Alloc;
};

// Parsed command-line arguments
struct CopyConfig {
     ....
};

// COFF specific details
struct MachOCopyConfig {
};

// ELF-specific details
struct ELFCopyConfig {
};

// MachO specific details
struct MachOCopyConfig {
};

struct Configuration {
    CopyConfig Common;
    Optional<ELFCopyConfig> ELF;
    Optional<MachOCopyConfig> MachO;
    Optional<COFFCopyConfig> COFF;
};

Somehow I find it a tiny bit more straightforward / simpler.

And have

Error executeObjcopyOnObjcopy(const Configuration &Config);

etc.

CC: @jhenderson

[jhenderson]

I don't have a strong feeling about this either way, if I'm honest. I'm happy for alternative approaches, if @alexshap prefers.

[seiya]

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

93 ↗

(On Diff #220054)

Eliminating duplicated references to CopyConfig sounds better to me. I'll adopt this design if there are no objections.

[MaskRay]

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

93 ↗

(On Diff #220054)

I don't have strong opinions on the interface (or I probably should say this really depends on the implementation)

No matter what design this patch ends up with, I hope we can avoid the verbose`Config.Common.Option` and use the simpler Config.Option, Common.Option or ELF.Option.

[alexander-shaposhnikov]

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

93 ↗

(On Diff #220054)

struct DriverConfig {
  SmallVector<Configuration, 1> Configurations; 
  BumpPtrAllocator Alloc;
};

// COFF specific details
struct MachOCopyConfig {
};

// ELF-specific details
struct ELFCopyConfig {
};

// MachO specific details
struct MachOCopyConfig {
};

struct CopyConfig {
   ...
   Optional<ELFCopyConfig> ELF;
   Optional<MachOCopyConfig> MachO;
   Optional<COFFCopyConfig> COFF;
};

[jakehehrlich]

What about having ELFcopyConfig inherit from CopyConfig?

[seiya]

In D67139#1670164, @jakehehrlich wrote:

What about having ELFcopyConfig inherit from CopyConfig?

How about this design? I believe moving the CopyConfig is not a big cost.

/// Parses CopyConfig and creates a ELFCopyConfig, or returns Error on parse errors.
class ELFCopyConfigBuilder {
  CopyConfig Common;
public:
  ELFCopyConfig(CopyConfig&& Common) : Common(std::move(Common)){};
  Expected<ELFCopyConfig> build();
}

class ELFCopyConfig : public ELFConfig {
public:
  Optional<uint8_t> NewSymbolVisibility;
  std::vector<NewSymbolInfo> SymbolsToAdd;

  ELFCopyConfig(CopyConfig&& Common, Optional<uint8_t> NewSymbolVisibility, /* ... */)
                : CopyConfig(std::move(Common)), NewSymbolVisibility(NewSymbolVisibility) /* ... */ {}
};

[jhenderson]

In D67139#1670164, @jakehehrlich wrote:

What about having ELFcopyConfig inherit from CopyConfig?

I thought of that, but if I'm not mistaken, that won't help us when we have a mixture of file formats, e.g. ELF and COFF. That could be a problem in some corner cases (e.g. archives containing objects of different formats). I think Seiya's one has the same problem - we can only use the common config once, not multiple times. If it's not a big deal to create copies of CopyConfig, then I don't mind doing it using inheritance: construct an ELFCopyConfig from the CopyConfig, but not std::move it. That would allow the original CopyConfig to continue to be used later.

[alexander-shaposhnikov]

for what it's worth - i was thinking about it for some time,
it seems to me that using inheritance here would look more complicated + would require a bit more code.
Somehow it feels better to me when there is only one instance of CopyConfig (and it's not moved around).
(yeah, some fields would be populated only when the format of the input is known, but overall this looks kinda simpler to me)
(but i don't want to insist)

[seiya]

Thanks you for feedbacks. Putting it all together, the @alexshap's suggestion (D67139#inline-607235) looks desirable to me because:

• It avoids verbose Config.Common.Option as @MaskRay pointed out in D67139#inline-607071.
• It supports a mixture of file formats as @jhenderson described in D67139#1671069 without additional memory copies of CopyConfig.

I'll update the patch but comments are still welcome of course.

[seiya]

• Adopt @alexshap's idea (D67139#inline-607235).

[seiya]

• Added a comment for parseELFConfig.

[MaskRay]

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

265 ↗

(On Diff #220640)

typedef -> using

[seiya]

• Addressed a review comment.

[jhenderson]

LGTM, thanks!

llvm/trunk/tools/llvm-objcopy/CMakeLists.txt

 add_llvm_tool(llvm-objcopy
   Buffer.cpp
   CopyConfig.cpp
   llvm-objcopy.cpp
   COFF/COFFObjcopy.cpp
   COFF/Object.cpp
   COFF/Reader.cpp
   COFF/Writer.cpp
+  ELF/ELFConfig.cpp
   ELF/ELFObjcopy.cpp
   ELF/Object.cpp
   MachO/MachOObjcopy.cpp
   MachO/MachOReader.cpp
   MachO/MachOWriter.cpp
   MachO/MachOLayoutBuilder.cpp
   MachO/Object.cpp
   DEPENDS

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

 //===- CopyConfig.h -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 #define LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 
+#include "ELF/ELFConfig.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Support/Allocator.h"

 public:
   void addMatcher(NameOrRegex Matcher) {
     Matchers.push_back(std::move(Matcher));
   }
   bool matches(StringRef S) const { return is_contained(Matchers, S); }
   bool empty() const { return Matchers.empty(); }
 };
 
-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;
-};
-
 // Configuration for copying/stripping a single file.
 struct CopyConfig {
+  // Format-specific options to be initialized lazily when needed.
+  Optional<elf::ELFCopyConfig> ELF;
+
   // Main input/output options
   StringRef InputFilename;
   FileFormat InputFormat;
   StringRef OutputFilename;
   FileFormat OutputFormat;
 
   // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
   Optional<MachineInfo> OutputArch;
 
   // Advanced options
   StringRef AddGnuDebugLink;
   // Cached gnu_debuglink's target CRC
   uint32_t GnuDebugLinkCRC32;
   StringRef BuildIdLinkDir;
   Optional<StringRef> BuildIdLinkInput;
   Optional<StringRef> BuildIdLinkOutput;
   Optional<StringRef> ExtractPartition;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
   StringRef AllocSectionsPrefix;
   DiscardType DiscardMode = DiscardType::None;
-  Optional<uint8_t> NewSymbolVisibility;
+  Optional<StringRef> NewSymbolVisibility;
 
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
-  std::vector<NewSymbolInfo> SymbolsToAdd;
+  std::vector<StringRef> SymbolsToAdd;
 
   // Section matchers
   NameMatcher KeepSection;
   NameMatcher OnlySection;
   NameMatcher ToRemove;
 
   // Symbol matchers
   NameMatcher SymbolsToGlobalize;
struct CopyConfig {
   bool StripDWO = false;
   bool StripDebug = false;
   bool StripNonAlloc = false;
   bool StripSections = false;
   bool StripUnneeded = false;
   bool Weaken = false;
   bool DecompressDebugSections = false;
   DebugCompressionType CompressionType = DebugCompressionType::None;
+
+  // parseELFConfig performs ELF-specific command-line parsing. Fills `ELF` on
+  // success or returns an Error otherwise.
+  Error parseELFConfig() {
+    if (!ELF) {
+      Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this);
+      if (!ELFConfig)
+        return ELFConfig.takeError();
+      ELF = *ELFConfig;
+    }
+    return Error::success();
+  }
 };
 
 // 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.
 struct DriverConfig {
   SmallVector<CopyConfig, 1> CopyConfigs;
   BumpPtrAllocator Alloc;

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

parseSetSectionFlagValue(StringRef FlagValue) {
   Expected<SectionFlag> ParsedFlagSet = parseSectionFlagSet(SectionFlags);
   if (!ParsedFlagSet)
     return ParsedFlagSet.takeError();
   SFU.NewFlags = *ParsedFlagSet;
 
   return SFU;
 }
 
-static Expected<NewSymbolInfo> parseNewSymbolInfo(StringRef FlagValue,
-                                                  uint8_t DefaultVisibility) {
-  // 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>.
-  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)>;
-  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("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", [] {})
-            .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;
-}
-
 struct TargetInfo {
   FileFormat Format;
   MachineInfo Machine;
 };
 
 // FIXME: consolidate with the bfd parsing used by lld.
 static const StringMap<MachineInfo> TargetMap{
     // Name, {EMachine, 64bit, LittleEndian}
Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   // 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 (opt::Arg *A = InputArgs.getLastArg(OBJCOPY_new_symbol_visibility)) {
-    const uint8_t Invalid = 0xff;
-    Config.NewSymbolVisibility = StringSwitch<uint8_t>(A->getValue())
-                                     .Case("default", ELF::STV_DEFAULT)
-                                     .Case("hidden", ELF::STV_HIDDEN)
-                                     .Case("internal", ELF::STV_INTERNAL)
-                                     .Case("protected", ELF::STV_PROTECTED)
-                                     .Default(Invalid);
-
-    if (Config.NewSymbolVisibility == Invalid)
-      return createStringError(
-          errc::invalid_argument, "'%s' is not a valid symbol visibility",
-          InputArgs.getLastArgValue(OBJCOPY_new_symbol_visibility).str().c_str());
-  }
+  if (opt::Arg *A = InputArgs.getLastArg(OBJCOPY_new_symbol_visibility))
+    Config.NewSymbolVisibility =
+        InputArgs.getLastArgValue(OBJCOPY_new_symbol_visibility);
 
   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;
if (Error E = addSymbolsFromFile(Config.UnneededSymbolsToRemove, DC.Alloc,
                                      Arg->getValue(), UseRegex))
       return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.addMatcher({Arg->getValue(), UseRegex});
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbols))
     if (Error E = addSymbolsFromFile(Config.SymbolsToKeep, DC.Alloc,
                                      Arg->getValue(), UseRegex))
       return std::move(E);
-  for (auto Arg : InputArgs.filtered(OBJCOPY_add_symbol)) {
-    Expected<NewSymbolInfo> NSI = parseNewSymbolInfo(
-        Arg->getValue(),
-        Config.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT));
-    if (!NSI)
-      return NSI.takeError();
-    Config.SymbolsToAdd.push_back(*NSI);
-  }
+  for (auto Arg : InputArgs.filtered(OBJCOPY_add_symbol))
+    Config.SymbolsToAdd.push_back(Arg->getValue());
 
   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/trunk/tools/llvm-objcopy/ELF/ELFConfig.h

+//===- ELFConfig.h ----------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_OBJCOPY_ELFCONFIG_H
+#define LLVM_TOOLS_OBJCOPY_ELFCONFIG_H
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Object/ELFTypes.h"
+#include "llvm/Support/Error.h"
+#include <vector>
+
+namespace llvm {
+namespace objcopy {
+struct CopyConfig;
+
+namespace elf {
+
+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;
+};
+
+struct ELFCopyConfig {
+  Optional<uint8_t> NewSymbolVisibility;
+  std::vector<NewSymbolInfo> SymbolsToAdd;
+};
+
+Expected<ELFCopyConfig> parseConfig(const CopyConfig &Config);
+
+} // namespace elf
+} // namespace objcopy
+} // namespace llvm
+
+#endif

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

+//===- ELFConfig.cpp ------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CopyConfig.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
+namespace llvm {
+namespace objcopy {
+namespace elf {
+
+static Expected<NewSymbolInfo> parseNewSymbolInfo(StringRef FlagValue,
+                                                  uint8_t DefaultVisibility) {
+  // 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>.
+  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)>;
+  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("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", [] {})
+            .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<ELFCopyConfig> parseConfig(const CopyConfig &Config) {
+  ELFCopyConfig ELFConfig;
+  if (Config.NewSymbolVisibility) {
+    const uint8_t Invalid = 0xff;
+    ELFConfig.NewSymbolVisibility =
+        StringSwitch<uint8_t>(*Config.NewSymbolVisibility)
+            .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",
+                               Config.NewSymbolVisibility->str().c_str());
+  }
+
+  for (StringRef Arg : Config.SymbolsToAdd) {
+    Expected<elf::NewSymbolInfo> NSI = parseNewSymbolInfo(
+        Arg,
+        ELFConfig.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT));
+    if (!NSI)
+      return NSI.takeError();
+    ELFConfig.SymbolsToAdd.push_back(*NSI);
+  }
+
+  return ELFConfig;
+}
+
+} // end namespace elf
+} // end namespace objcopy
+} // end namespace llvm

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

for (const auto &Flag : Config.DumpSection) {
     if (Error E = dumpSectionToFile(SecName, File, Obj))
       return E;
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
-  for (const NewSymbolInfo &SI : Config.SymbolsToAdd) {
+  for (const NewSymbolInfo &SI : Config.ELF->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);
   }
 
   if (Config.EntryExpr)
Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
   if (Error E = handleArgs(Config, *Obj, Reader, OutputElfType))
     return E;
   return writeOutput(Config, *Obj, Out, OutputElfType);
 }
 
 Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 Buffer &Out) {
   uint8_t NewSymbolVisibility =
-      Config.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
+      Config.ELF->NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
   BinaryReader Reader(&In, NewSymbolVisibility);
   std::unique_ptr<Object> Obj = Reader.create();
 
   // 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, *Obj, Reader, OutputElfType))

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

for (const NewArchiveMember &Member : NewMembers) {
     if (Error E = FB.commit())
       return E;
   }
   return Error::success();
 }
 
 /// The function executeObjcopyOnIHex does the dispatch based on the format
 /// of the output specified by the command line options.
-static Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
+static Error executeObjcopyOnIHex(CopyConfig &Config, MemoryBuffer &In,
                                   Buffer &Out) {
   // TODO: support output formats other than ELF.
+  if (Error E = Config.parseELFConfig())
+    return E;
   return elf::executeObjcopyOnIHex(Config, In, Out);
 }
 
 /// The function executeObjcopyOnRawBinary does the dispatch based on the format
 /// of the output specified by the command line options.
-static Error executeObjcopyOnRawBinary(const CopyConfig &Config,
-                                       MemoryBuffer &In, Buffer &Out) {
+static Error executeObjcopyOnRawBinary(CopyConfig &Config, MemoryBuffer &In,
+                                       Buffer &Out) {
   switch (Config.OutputFormat) {
   case FileFormat::ELF:
   // FIXME: Currently, we call elf::executeObjcopyOnRawBinary even if the
   // output format is binary/ihex or it's not given. This behavior differs from
   // GNU objcopy. See https://bugs.llvm.org/show_bug.cgi?id=42171 for details.
   case FileFormat::Binary:
   case FileFormat::IHex:
   case FileFormat::Unspecified:
+    if (Error E = Config.parseELFConfig())
+      return E;
     return elf::executeObjcopyOnRawBinary(Config, In, Out);
   }
 
   llvm_unreachable("unsupported output format");
 }
 
 /// The function executeObjcopyOnBinary does the dispatch based on the format
 /// of the input binary (ELF, MachO or COFF).
-static Error executeObjcopyOnBinary(const CopyConfig &Config,
-                                    object::Binary &In, Buffer &Out) {
-  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In))
+static Error executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In,
+                                    Buffer &Out) {
+  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
+    if (Error E = Config.parseELFConfig())
+      return E;
     return elf::executeObjcopyOnBinary(Config, *ELFBinary, Out);
-  else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))
+  } else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))
     return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
   else if (auto *MachOBinary = dyn_cast<object::MachOObjectFile>(&In))
     return macho::executeObjcopyOnBinary(Config, *MachOBinary, Out);
   else
     return createStringError(object_error::invalid_file_type,
                              "unsupported object file format");
 }
 
-static Error executeObjcopyOnArchive(const CopyConfig &Config,
-                                     const Archive &Ar) {
+static Error executeObjcopyOnArchive(CopyConfig &Config, const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       return createFileError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);
 
   return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type
 /// of input (raw binary, archive or single object file) and takes care of the
 /// format-agnostic modifications, i.e. preserving dates.
-static Error executeObjcopy(const CopyConfig &Config) {
+static Error executeObjcopy(CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.InputFilename != "-") {
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       return createFileError(Config.InputFilename, EC);
   } else {
     Stat.permissions(static_cast<sys::fs::perms>(0777));
   }
 
-  typedef Error (*ProcessRawFn)(const CopyConfig &, MemoryBuffer &, Buffer &);
+  using ProcessRawFn = Error (*)(CopyConfig &, MemoryBuffer &, Buffer &);
   ProcessRawFn ProcessRaw;
   switch (Config.InputFormat) {
   case FileFormat::Binary:
     ProcessRaw = executeObjcopyOnRawBinary;
     break;
   case FileFormat::IHex:
     ProcessRaw = executeObjcopyOnIHex;
     break;
int main(int argc, char **argv) {
   Expected<DriverConfig> DriverConfig =
       IsStrip ? parseStripOptions(Args, reportWarning)
               : parseObjcopyOptions(Args);
   if (!DriverConfig) {
     logAllUnhandledErrors(DriverConfig.takeError(),
                           WithColor::error(errs(), ToolName));
     return 1;
   }
-  for (const CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
+  for (CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
     if (Error E = executeObjcopy(CopyConfig)) {
       logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
       return 1;
     }
   }
 
   return 0;
 }