[llvm-objcopy] Refactor CopyConfig structure.

[avl]

Summary

This patch prepares llvm-objcopy to move its implementation
into a separate library. To make it possible it is necessary
to minimize internal dependencies. Current CopyConfig has the following
drawbacks:

1. Current CopyConfig has parsed and unparsed options at the same time. Thus, parsing functionality is located in different places. Instead, It would be good to parse options once and then have parsed options inside CopyConfig. This patch does all parsing job in the parse*() routines.

2. Current implementation has an attempt to hide format-specific options. This patch makes it in a more general way: separate format-specific options using subclasses and allow to create the format-specific views at all options.
   
   So, this patch creates some interfaces representing common and format-specific options:
   
   CopyConfigBase CopyConfigELF CopyConfigCOFF CopyConfigMachO CopyConfigWasm
   
   And a class that allows to switch the view on the full options set:
   
   MultiFormatCopyConfig.

[jhenderson]

It also makes format of error mesages for --add-symbol option matching with format of error mesages for other options.

Could you do this in a separate patch upfront, please? As things stand, this patch is not NFC, because of this...! I have some concerns about the specific nature of the non-NFC part, which I'd like to comment on separately.

As for the main part of this change, I think this is essentially trying to redo D67139, which had a lot of back and forth discussion on it. Apart from anything else, the *Objcopy.cpp files are not where to put command-line parsing logic, which was an item from that review. Please go over the comments from D67139, and make sure to take the points raised in the comments on board. Once you have done that, feel free to propose a new solution (which could be similar to this, or something else entirely), if you feel it is important.

[avl]

Could you do this in a separate patch upfront, please? As things stand, this patch is not NFC, because of this...! I have some concerns about the specific nature of the non-NFC part, which I'd like to comment on separately.

It would be a bit artificial... Since the change on how error message is displayed happens because option processing was moved form common part into the specific part. i.e. it could not be separated from this change. But I might be done by adding extra code:

Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this);
if (!ELFConfig)
  return createFileError(Config.InputFilename, ELFConfig.takeError());

Would create a separate review.

[avl]

As for the main part of this change, I think this is essentially trying to redo D67139, which had a lot of back and forth discussion on it. Apart from anything else, the *Objcopy.cpp files are not where to put command-line parsing logic, which was an item from that review. Please go over the comments from D67139, and make sure to take the points raised in the comments on board. Once you have done that, feel free to propose a new solution (which could be similar to this, or something else entirely), if you feel it is important.

Thanks, will look through it and get back.

[avl]

addressed comments:

Restored ELFConfig.h/cpp as a separate file keeping options parsing code.
Left usage of parsing code of elf-specific options inside /ELF directory.

D67139 suggested parsing specific options lazily.
i.e. To parse them only in case when they are necessary.
But it places format-specific options in a common CopyConfig structure.
This does not look good since all users of ELFConfig are inside of
/ELF directory(Thus we do not need to make them visible to others).
This patch hides elf-specific processing inside /ELF directory
and parses format-specific options lazily.

[avl]

Looking at the design for ELF parseable options more, I do not really understand why parsing of ELF options should be delayed. It seems that they might be set at the same time when all other options parsed(i.e. as it was before D67139):

remove

std::vector<StringRef> SymbolsToAdd;
Optional<StringRef> NewSymbolVisibility

from CopyConfig. So that CopyConfig never contains unparsed strings.

add specific ELF fields to CopyConfig:

CopyConfig {

  // ELF specific options.
  Optional<uint8_t> NewSymbolVisibility;
  std::vector<NewSymbolInfo> SymbolsToAdd;
}

parseObjcopyOptions() {
  // set NewSymbolVisibility and SymbolsToAdd
  // if corresponding options exist in arguments.
}

What do you think?

[jhenderson]

In D99055#2644077, @avl wrote:

addressed comments:

Restored ELFConfig.h/cpp as a separate file keeping options parsing code.
Left usage of parsing code of elf-specific options inside /ELF directory.

D67139 suggested parsing specific options lazily.
i.e. To parse them only in case when they are necessary.
But it places format-specific options in a common CopyConfig structure.
This does not look good since all users of ELFConfig are inside of
/ELF directory(Thus we do not need to make them visible to others).
This patch hides elf-specific processing inside /ELF directory
and parses format-specific options lazily.

Lazy parsing previously also had the advantage that the parsing only needed to happen once at most, even if there were multiple input files. It looks like this change causes the ELF specific config to be calculated once per input, which seems suboptimal? Aside from that, this looks like a general improvement to me. What do others think?

[avl]

after some more thinking, it seems to me that nor this patch nor current llvm version is not a right solution.

Current CopyConfig has parsed and unparsed options at the same time - this is not good in general.
Unparsed ELF options are visible to other formats.
Parsed ELF options are visible to other formats.
Delayed options parsing makes CopyConfig to be invalid for some time periods(until target options are parsed).
Current CopyConfig might be in inconsistent state (if several target would be specified at the same time).

I think that the better way would be something like this:

CopyConfig has all options in parsed state. It is filled in parseObjcopyOptions().

CopyConfig {
   StringRef InputFilename;

  // ELF specific options.
  Optional<uint8_t> NewSymbolVisibility;
  std::vector<NewSymbolInfo> SymbolsToAdd;
}

Target ElfCopyConfig has only options which are supported by the target.

ElfCopyConfig {
   StringRef InputFilename;

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

Error executeObjcopyOnBinary(const ElfCopyConfig &Config,
                             object::ELFObjectFileBase &In, raw_ostream &Out)

CopyConfig has methods converting it to the target config:

CopyConfig {
  ElfCopyConfig getELFConfig();
}

static Error executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In,
                                    raw_ostream &Out) {
  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
    return elf::executeObjcopyOnBinary(Config.getELFConfig(), *ELFBinary, Out);
}

Would implement that approach and show for review.

[avl]

Changed a patch to solve following problems:

1. Current CopyConfig has parsed and unparsed options at the same time. Thus, parsing functionality is located in different places. Instead, It would be good to parse options once and then have parsed options inside CopyConfig. This patch does all parsing job in the parse*() routines.
2. Current trunc implementation has an attempt to hide format-specific options. This patch makes it in a more general way: separate format-specific options using subclasses and allow to create the format-specific views at all options.
   
   So, this patch creates some interfaces representing common and format-specific options:
   
   CopyConfigBase CopyConfigELF CopyConfigCOFF CopyConfigMachO CopyConfigWasm
   
   And a class that allows to switch the view on the full options set:
   
   MultiFormatCopyConfig.
   
   I am a bit unsure whether it is finally good solution. From one side, It creates a good separation of concerns: clearly categorize options and separate them from other formats. From other side, the implementation looks a bit complicated.
   
   @jhenderson @alexshap @MaskRay What is your opinion?

[jhenderson]

Thanks for the update. I do have some concerns about the multiple inheritance stuff going on, if I'm honest, as usually this is a design smell, but I haven't got the time right now to think about it. Unfortunately, I'm not likely to get more of a chance to later this week either, as from Thursday I'm off for just over two weeks.

I do still think you should normalise the error messages in a separate prerequisite change. Small changes are good, especially if it helps make something else a pure refactor.

If I don't come back around to this before I go, please feel free to go with the approval of others. I'm resigning from reviewing to remove my "request changes" flag, but I may come back to it later.

[dblaikie]

In D99055#2655081, @jhenderson wrote:

Thanks for the update. I do have some concerns about the multiple inheritance stuff going on, if I'm honest, as usually this is a design smell

+1 I can take a look more closely to try to tease out whether there's some alternative that might be nicer, but maybe @rupprecht or @MaskRay might have more context here already. (at a glance the use of virtual inheritance here seems plausible - since these are just structs - if all the config property access went through virtual functions then it could probably be done without virtual inheritance, but with virtual functions and maybe some CRTP mix-in kind of implementations, but that may not be any better - but I wouldn't mind a few other eyes on this, maybe there's some other ways forward here)

I do still think you should normalise the error messages in a separate prerequisite change. Small changes are good, especially if it helps make something else a pure refactor.

Sounds pretty reasonable.

[avl]

I do have some concerns about the multiple inheritance stuff going on, if I'm honest, as usually this is a design smell

The idea here is to reuse only data. The most weaknesses of multiplue inheritance in C++
came with handling of inherited methods. virtual inheritance of data members looks
safe(since they could not be redefined). Though, copy and move operators should be properly addressed.
But if there is something bad with this approach I am open to change it.

I do still think you should normalise the error messages in a separate prerequisite change. Small changes are good, especially if it helps make something else a pure refactor.

will create a separate review.

[avl]

if all the config property access went through virtual functions then it could probably be done without virtual inheritance, but with virtual functions and maybe some CRTP mix-in kind of implementations, but that may not be any better

I thought about similar approach : virtual functions and maybe some CRTP mix-in kind of implementations.
But it looked much more heavyweight(at least the implementation which I was thinking of).

struct CopyConfig {};

template < class T >
struct BaseMixin : public T {
  virtual int getCommonOpt() = 0;
};

template < class T >
struct ELFMixin : public T {
  virtual int getElfOpt() = 0;
};

template < class T >
struct COFFMixin : public T {};

template < class T >
struct MachOMixin : public T {};

template < class T >
struct WasmMixin : public T {};


struct CopyConfigELF : public BaseMixin<CopyConfig>,
                       public ELFMixin<CopyConfig> {};

struct CopyConfigCOFF : public BaseMixin<CopyConfig>,
                        public COFFMixin<CopyConfig> {};

struct CopyConfigMachO : public BaseMixin<CopyConfig>,
                         public MachOMixin<CopyConfig> {};

struct CopyConfigWasm : public BaseMixin<CopyConfig>,
                        public WasmMixin<CopyConfig> {};
                        
struct CopyConfigFull : public BaseMixin<CopyConfig>,
                        public ELFMixin<CopyConfig>,
                        public COFFMixin<CopyConfig>,
                        public MachOMixin<CopyConfig>,
                        public WasmOMixin<CopyConfig>
                         {}


struct MultiFormatCopyConfig  {

  MultiFormatCopyConfig() : ElfProxy(Opts) {
  }
                               
  virtual const CopyConfigELF & getELFConfig() const {
      return ElfProxy;
  }

  protected:
  class CopyConfigELFProxy : public CopyConfigELF{
    CopyConfigELFProxy(CopyConfigFull& Opts) : Opts(Opts){}
    int getElfOpt() override {
      return Opts.getElfOpt();
    }
     int getCommonOpt() override {
      return Opts.getCommonOpt();
     }
    CopyConfigFull& Opts;
  };


  CopyConfigFull Opts;
  CopyConfigELFProxy ElfProxy;
}

[dblaikie]

So the general point of this change is that previously all the options were in one struct which meant that, say, the MachO implementation could accidentally end up using an ELF option value, etc? And this way the implementations can accept the common options (the base class) and their implementation-specific options?

Any sense of whether that sort of cross-use was/is a significant source of bugs, or whether it'd be OK to just have chunks of the config specify "these are for ELF", "these are for MachO", etc (just in comments, or perhaps using ELF/MachO as a prefix, or having them in an ELF/MachO/etc subobject - while still having them all together/without the ability to create a common-and-MachO-view, common-and-ELF-view, etc) - I guess this came out of discussions in other reviews around the "it is necessary to minimize internal dependencies."? Might be good to have more of that context here to motivate/explain the benefits.

[avl]

In D99055#2660459, @dblaikie wrote:

So the general point of this change is that previously all the options were in one struct which meant that, say, the MachO implementation could accidentally end up using an ELF option value, etc? And this way the implementations can accept the common options (the base class) and their implementation-specific options?

Any sense of whether that sort of cross-use was/is a significant source of bugs, or whether it'd be OK to just have chunks of the config specify "these are for ELF", "these are for MachO", etc (just in comments, or perhaps using ELF/MachO as a prefix, or having them in an ELF/MachO/etc subobject - while still having them all together/without the ability to create a common-and-MachO-view, common-and-ELF-view, etc) - I guess this came out of discussions in other reviews around the "it is necessary to minimize internal dependencies."? Might be good to have more of that context here to motivate/explain the benefits.

my immediate need(driven by D88827) is to remove dependencies to the parts which are not suitable for the library and to remove things that complicate the library interface.
In the current llvm implementation these are :

1. CopyConfig has two versions of some options. Parsed and unparsed. f.e. These are SymbolsToAdd and ELF.SymbolsToAdd. It is redundant for the library interface to have two of them.

2. Current implementation uses lazy initialization of CopyConfig.ELF. i.e. it parses ELF-specific options later than others. It would be inconvenient to have potentially uninitialized parts in the library interface. It is also would be incorrect to initialize this part inside a library(Since the library should not depend on the user interface of the concrete tool)

another thing is that D67139 suggested dividing options into format-specific parts. Thus this patch tries to resolve points 1 and 2 and to follow D67139 idea. Speaking
of whether such format-specific grouping is necessary and whether it is worth efforts: I think that probably it is not a big problem that format specific parts would see
other options(it would be nice to have, but if it requires many things to do then we might leave without it). So to resolve 1 and 2 it would be just fine to have:

struct CopyConfig {
  // common options 
  StringRef InputFilename;
  ...
  
  // ELF specific options
  std::vector<NewSymbolInfo> SymbolsToAdd;
  
  // MachO specific options
  bool KeepUndefined = false; 
};

When that structure would be created, all input options would be put in CopyConfig in the parsed state.
Later this structure would be passed into format-specific part:

Error elf::executeObjcopyOnBinary(const CopyConfig &Config,
                             object::Binary &In, raw_ostream &Out);

That solution would be fine from the point of view "preparation to move objcopy implementation to the separate library". But it would revert the idea done by D67139.

[avl]

So the general point of this change is that previously all the options were in one struct which meant that, say, the MachO implementation could accidentally end up using an ELF option value, etc? And this way the implementations can accept the common options (the base class) and their implementation-specific options?

My understanding is that above is point which D67139 suggested. This patch(D99055) tried to preserve it and implemented using other design approach.

Any sense of whether that sort of cross-use was/is a significant source of bugs, or whether it'd be OK to just have chunks of the config specify "these are for ELF", "these are for MachO", etc (just in comments, or perhaps using ELF/MachO as a prefix, or having them in an ELF/MachO/etc subobject - while still having them all together/without the ability to create a common-and-MachO-view, common-and-ELF-view, etc) - I guess this came out of discussions in other reviews around the "it is necessary to minimize internal dependencies."?

I think we do not have many bugs connected with cross-use. From that point of view it is easier and might be OK to use simpler approach ("just have chunks of the config specify "these are for ELF", "these are for MachO", ...").

From the point of view of clear design it is better to have solution which will avoid cross usages. We might use a solution from this patch(D99055) or delay it until better solution would be found.

What would be the better ?

[avl]

The one more argument for design presented by this patch is that: it is assumed that part of the interface
would be moved into the future ObjCopy library. The part which would be moved is quite small and clear.
Second part would be left in the llvm-objcopy tool. If we do not like its implementation(ParsedMultiFormatConfig)
we still might be able to reimplement llvm-objcopy part using another approach, but the library interface
will remain the same. Currently, indeed, we do not have many cross-format bugs. But, when this code
would be done as a library then it becomes more possibilities to do such cross-format bugs.
So, it is generally helpful if the interface of the library would prevent cross-format bugs.
I refactored this patch so that it would be more clear which part is supposed to go into the library
and which part is supposed to be left inside llvm-objcopy:

library part:

CopyConfigBase.h
CopyConfigELF.h
CopyConfigCOFF.h
CopyConfigMachO.h
CopyConfigWasm.h
MultiFormatCopyConfig.h

llvm-objcopy tool part:

ParsedConfig.cpp

[avl]

ping.

[avl]

ping. Colleagues, what is your opinion on the way how this refactoring should be done?

I think the solution presented in this patch is a good way, especially from the library interface perspective.
It separates options and passes only relevant options to format specific parts. So my suggestion is to integrate this patch.

But if that approach is not good I am fine to implement a simpler approach: when all options are in the same structure, the structure is divided into the chunks of the config(by comments) "these are for ELF", "these are for MachO".

[dblaikie]

In D99055#2688559, @avl wrote:

ping. Colleagues, what is your opinion on the way how this refactoring should be done?

I think the solution presented in this patch is a good way, especially from the library interface perspective.
It separates options and passes only relevant options to format specific parts. So my suggestion is to integrate this patch.

But if that approach is not good I am fine to implement a simpler approach: when all options are in the same structure, the structure is divided into the chunks of the config(by comments) "these are for ELF", "these are for MachO".

I think @jhenderson is out of office for a bit - hoping when he's back he can weigh in on some of this. Give it another week or two hopefully.

[jhenderson]

I'm back from my time off, although have a massive backlog to work my way through, so haven't had too much time to revisit the history of this and related patches, so far. I took a step back and was wondering whether another approach would be significantly simpler. There's likely problems with my below suggesting, which may have already been covered elsewhere, but hopefully any explanations of why any issues are indeed issues will help me understand things better. Apologies if you've addressed some of the issues before.

Basically, the idea is to change the executeObjcopyOn* functions to take two separate configs, a base one and a format-specific one. Each file format would (where necessary) have a simple struct that contains the format-specific values, and then the base one would have the options shared by more than one format. This would of course require updating all references to the Config to reference the correct version as required, but if it leads to a nicer design overall, I think it is worth it.

To assist with the lazy parsing, a factory-like class could be responsible for providing the appropriate config when requested. The code might like something like this:

class ConfigManager {
public:
  ConfigManager(/*...*/); // Constructs Common. Stores raw argument data needed to construct other configs.

  const CommonConfig &getConfig() const { return Common; }
  const ELFConfig &getELFConfig() {
    if (!Elf)
      Elf = ...;
    return *Elf;
  }
  /* Same for other formats. */
  
private:
  CommonConfig Common;
  Optional<COFFConfig> Coff;
  Optional<ELFConfig> Elf;
  Optional<MachOConfig> MachO;
  Optional<WasmConfig> Wasm;
};

static Error executeObjcopyOnBinary(ConfigManager &Configs, object::Binary &In,
                                    raw_ostream &Out) {
  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
    if (Error E = Config.parseELFConfig())
      return E;
    return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
  }
  /* etc */
}

What do you think?

[avl]

What do you think?

This solution has some concerns caused by lazy options parsing.
I do not understand whether/why we need lazy parsing.
Thus, depending on the reason for lazy parsing, these concerns might be acceptable price or might be not. To support lazy parsing it is necessary
to keep unparsed data, move some parsing functionality into parseELFConfig().
In case it would be necessary to create another tool(llvm-another-tool) then
we need to implement a child of ConfigManager and implement another version of
parseELFConfig(). All this is doable but looks like unnecessary complications.
If it would be not necessary to support lazy parsing then the solution would look simpler.
Do we need to support lazy parsing?

The same solution without lazy parsing looks simpler:

class ConfigManager {
public:
  ConfigManager(/*...*/); // initializes all options into parsed state.

  const CommonConfig &getConfig() const { return Common; }
  const ELFConfig &getELFConfig() const { return Elf; }
  /* Same for other formats. */
  
private:
  CommonConfig Common;
  COFFConfig   Coff;
  ELFConfig    Elf;
  MachOConfig  MachO;
  WasmConfig   Wasm;
};

static Error executeObjcopyOnBinary(ConfigManager &Configs, object::Binary &In,
                                    raw_ostream &Out) {
  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In))
    return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
  /* etc */
}

[jhenderson]

In D99055#2699528, @avl wrote:

What do you think?

This solution has some concerns caused by lazy options parsing.
I do not understand whether/why we need lazy parsing.
Thus, depending on the reason for lazy parsing, these concerns might be acceptable price or might be not. To support lazy parsing it is necessary
to keep unparsed data, move some parsing functionality into parseELFConfig().
In case it would be necessary to create another tool(llvm-another-tool) then
we need to implement a child of ConfigManager and implement another version of
parseELFConfig(). All this is doable but looks like unnecessary complications.
If it would be not necessary to support lazy parsing then the solution would look simpler.
Do we need to support lazy parsing?

I've not checked, but if memory serves me correctly, there are some options (--add-symbol, I think, at least) that are parsed differently depending on the file format. As such, we can't parse them upfront - if we did, one of the two parse*Config functions would fail. We'd only want that to happen if a user was providing both file formats and the option. Additionally, we don't want to warn about unsupported options for specific file formats, if we don't use those file formats.

[avl]

I've not checked, but if memory serves me correctly, there are some options (--add-symbol, I think, at least) that are parsed differently depending on the file format. As such, we can't parse them upfront - if we did, one of the two parse*Config functions would fail. We'd only want that to happen if a user was providing both file formats and the option.

It seems the same problem might occur even with lazy parsing. What if we have an archive containing object files with different formats. We might not be able to specify "--add-symbol" option which would be successfully parsed for both of them:

if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
  if (Error E = Config.parseELFConfig()) <<<< one of this parsing routines might fail because of wrong format in source unparsed string.
    return E;
  return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
} else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In)) {
  if (Error E = Config.parseCOFFConfig())  <<<< one of this parsing routines might fail because of wrong format in source unparsed string.
    return E;
  return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
}

Probably we should avoid options having the same name but parsed differently for various formats?

[jhenderson]

In D99055#2700994, @avl wrote:

I've not checked, but if memory serves me correctly, there are some options (--add-symbol, I think, at least) that are parsed differently depending on the file format. As such, we can't parse them upfront - if we did, one of the two parse*Config functions would fail. We'd only want that to happen if a user was providing both file formats and the option.

It seems the same problem might occur even with lazy parsing. What if we have an archive containing object files with different formats. We might not be able to specify "--add-symbol" option which would be successfully parsed for both of them:

if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
  if (Error E = Config.parseELFConfig()) <<<< one of this parsing routines might fail because of wrong format in source unparsed string.
    return E;
  return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
} else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In)) {
  if (Error E = Config.parseCOFFConfig())  <<<< one of this parsing routines might fail because of wrong format in source unparsed string.
    return E;
  return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
}

Probably we should avoid options having the same name but parsed differently for various formats?

Yes, as mentioned, this is a problem if we have multiple inputs in different formats (including as archive members). I don't think we need to worry about encountering archives with different file format members though, as they will typically not be usable at link time, I'd think. If users want to use the option, they'll have to do it on the individual files in separate invocations.

Perhaps using a different option name would be useful, if we had that choice, but we don't always, either because of compatibility with existing tools, or to avoid unnecessarily breaking people's existing build scripts that use llvm-objcopy.

[avl]

Perhaps using a different option name would be useful, if we had that choice, but we don't always, either because of compatibility with existing tools, or to avoid unnecessarily breaking people's existing build scripts that use llvm-objcopy.

I see. will change the patch to follow this pattern:

class ConfigManager {
public:
  ConfigManager(/*...*/); // Constructs Common. Stores raw argument data needed to construct other configs.

  const CommonConfig &getConfig() const { return Common; }
  const ELFConfig &getELFConfig() {
    if (!Elf)
      Elf = ...;
    return *Elf;
  }
  /* Same for other formats. */
  
private:
  CommonConfig Common;
  Optional<COFFConfig> Coff;
  Optional<ELFConfig> Elf;
  Optional<MachOConfig> MachO;
  Optional<WasmConfig> Wasm;
};

static Error executeObjcopyOnBinary(ConfigManager &Configs, object::Binary &In,
                                    raw_ostream &Out) {
  if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
    if (Error E = Config.parseELFConfig())
      return E;
    return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
  }
  /* etc */
}

According to the separation by CommonConfig and ELFConfig: Would it be useful to separate options like in this patch:

struct CommonConfig {
  // Main input/output options
  StringRef InputFilename;
  FileFormat InputFormat = FileFormat::Unspecified;
  StringRef OutputFilename;
  FileFormat OutputFormat = FileFormat::Unspecified;

  // Advanced options
  StringRef AddGnuDebugLink;

  DiscardType DiscardMode = DiscardType::None;

  // Repeated options
  std::vector<StringRef> AddSection;
  std::vector<StringRef> DumpSection;

  // Section matchers
  NameMatcher OnlySection;
  NameMatcher ToRemove;

  // Symbol matchers
  NameMatcher SymbolsToRemove;
  NameMatcher UnneededSymbolsToRemove;

  // Map options
  StringMap<SectionFlagsUpdate> SetSectionFlags;
  StringMap<StringRef> SymbolsToRename;

  // Boolean options
  bool DeterministicArchives = true;
  bool OnlyKeepDebug = false;
  bool PreserveDates = false;
  bool StripAll = false;
  bool StripAllGNU = false;
  bool StripDebug = false;
  bool StripUnneeded = false;
}

struct ELFConfig {
  // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
  Optional<MachineInfo> OutputArch;

  // Advanced options
  // Cached gnu_debuglink's target CRC
  uint32_t GnuDebugLinkCRC32;
  Optional<StringRef> ExtractPartition;
  StringRef SplitDWO;
  StringRef SymbolsPrefix;
  StringRef AllocSectionsPrefix;
  Optional<uint8_t> NewSymbolVisibility;
  std::vector<NewSymbolInfo> SymbolsToAdd;

  // Section matchers
  NameMatcher KeepSection;

  // Map options
  StringMap<SectionRename> SectionsToRename;
  StringMap<uint64_t> SetSectionAlignment;

  // Symbol matchers
  NameMatcher SymbolsToGlobalize;
  NameMatcher SymbolsToKeep;
  NameMatcher SymbolsToLocalize;
  NameMatcher SymbolsToWeaken;
  NameMatcher SymbolsToKeepGlobal;

  // 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;

  // Boolean options
  bool AllowBrokenLinks = false;
  bool ExtractDWO = false;
  bool ExtractMainPartition = false;
  bool KeepFileSymbols = false;
  bool LocalizeHidden = false;
  bool StripDWO = false;
  bool StripNonAlloc = false;
  bool StripSections = false;
  bool Weaken = false;
  bool DecompressDebugSections = false;

  DebugCompressionType CompressionType = DebugCompressionType::None;
};

Or like in current llvm code :

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

?

[jhenderson]

Maybe it should be a two-part process? In this first patch, keep the split more-or-less the same as it is now, so that the diff is simpler, and we can focus on the more important aspects of the patch (switching to the new configuration structure), and then a second patch can move the options between structures so that they're in the most appropriate one.

[avl]

Implemented as suggested.

It seems to me that this version(using encapsulation instead of inheritance)
became more complex because of the necessity to repeatedly specify the name of the config
and to pass pair of configs. To minimize the number of changes we might use inheritance
like in this change(https://reviews.llvm.org/D99055?id=334944).
If the complexity is not a problem, then let`s use a variant from this last version of the patch.

[jhenderson]

I've skimmed bits of this briefly for now - I am quite busy and don't have the time for a more in-depth review at this point. It would be really helpful to get another pair of eyes on this approach too, to see what others think.

llvm/tools/llvm-objcopy/ConfigManager.cpp

709

Optional suggestion, which might help reduce the repetitive Config.Common and reduce the changes here:

ConfigManager ConfigMgr;
CommonConfig &Confg = ConfigMgr.Common;

The same could be done in other places too.

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

514

I'd be inclined to call these something like Config and ElfCfg (or even ElfConfig if you don't mind the hiding of the class type within this function). This reduces the changes needed here at least. Thinking about it, even if you want to name CommonConfig something reflecting the Common aspect, I'd call the variable CommonCfg not simply Common, and Elf similarly becomes ElfCfg. The reason is that Elf normally implies some sort of Elf object, not a config class. Renaming Common also helps describe its purpose more clearly.

[dblaikie]

In D99055#2716017, @jhenderson wrote:

I've skimmed bits of this briefly for now - I am quite busy and don't have the time for a more in-depth review at this point. It would be really helpful to get another pair of eyes on this approach too, to see what others think.

I've been watching, but my understanding is limited, and from what I've seen it doesn't seem like it's been that beneficial to try to split these things up when they all have to be known to the frontend anyway. So one big struct with some comments about which bits are for which architecture sounds like it would be OK to me - but perhaps I'm missing some things.

llvm/tools/llvm-objcopy/ConfigManager.h

27

Why is there an inheritance relationship here - if there's only one class that implements MultiFormatConfig - why isn't ConfigManager a non-virtual API?

[avl]

llvm/tools/llvm-objcopy/ConfigManager.h

27

MultiFormatConfig interface is used when passed to routine working with archives:

Expected<std::vector<NewArchiveMember>>
createNewArchiveMembers(const MultiFormatConfig &Config,
                        const object::Archive &Ar);

It would be bad to pass whole ConfigManager here since ConfigManager has elements not used by createNewArchiveMembers. Additionally ConfigManager is a llvm-objcopy specific thing. For some another tool(llvm-another-tool) there might be another implementation of config manager, but it needs to be presented as a universal interface for createNewArchiveMembers. MultiFormatConfig is a such interface.

[avl]

renamed variables as requested.

[dblaikie]

llvm/tools/llvm-objcopy/ConfigManager.h

27

It seems like an abstraction without a use case yet, so it should wait until there is a use case?

I understand some of this is refactoring to make this more amenable to be a general purpose library - but if all the uses of the archive support currently want a big multi-format config, it seems OK for that to be a single non-virtual thing at the moment. Then when someone has a need to build a format-specific tool with archive support (hopefully we don't do too much of that in LLVM - it's nice to be generic over all our supported object formats where possible) they could add this abstraction?

(what's the idea, that they could implement ConfigManager with a version that returns errors for everything other than the object format they support (ie: an ELF-only use of this would return a real ELF config, but getCOFF/MachO/WasmConfig would return Error about how those formats aren't supported)

[avl]

added several missed renamings.

[avl]

llvm/tools/llvm-objcopy/ConfigManager.h

27

It seems like an abstraction without a use case yet, so it should wait until there is a use case?

I understand some of this is refactoring to make this more amenable to be a general purpose library - but if all the uses of the archive support currently want a big multi-format config, it seems OK for that to be a single non-virtual thing at the moment. Then when someone has a need to build a format-specific tool with archive support (hopefully we don't do too much of that in LLVM - it's nice to be generic over all our supported object formats where possible) they could add this abstraction?

(what's the idea, that they could implement ConfigManager with a version that returns errors for everything other than the object format they support (ie: an ELF-only use of this would return a real ELF config, but getCOFF/MachO/WasmConfig would return Error about how those formats aren't supported)

My understanding is that there is a problem connected with the fact that there are options that could not be parsed properly without knowing the final format(the same options would be parsed differently for various formats). i.e. we could not parse&fill all options before we make a call to createNewArchiveMembers(). If such options are specified in command line and if archive contains files with different formats then we could not handle such option - it would fail for one or another format(https://reviews.llvm.org/D99055#2701025). Thus we assume that only one format might be requested.

The current solution is to have a callback from createNewArchiveMembers()(from the point where the format is known).
i.e. when it is known that the final format is ELF, we call getELFConfig() which parses lazy options and checks for unsupported options(using code from ConfigManager). Such a call back allows separating createNewArchiveMembers from the code which parses the source options. i.e. when llvm-objcopy code would be divided into the common library part and specific llvm-objcopy part then createNewArchiveMembers() and MultiFormatConfig will go into the library part. ConfigManager will stay in llvm-objcopy specific part. Without such separation, it would be necessary to put ConfigManager into the library part. Putting specific part into the common part looks wrong.

[jhenderson]

I think before you go too much further with this refactoring, it would be a good idea if we have subsequent patches ready to see which make use of this. Without a concrete use-case, we are making change for no particular reason. With the subsequent patches, we can then see whether the refactoring here and elsewhere actually makes sense for the final use-case.

[avl]

I think before you go too much further with this refactoring, it would be a good idea if we have subsequent patches ready to see which make use of this. Without a concrete use-case, we are making change for no particular reason. With the subsequent patches, we can then see whether the refactoring here and elsewhere actually makes sense for the final use-case.

https://reviews.llvm.org/D86539 is the example of possible use(it is a bit outdated, but still shows a usecase).
Please, consider following fragment:

template <class ELFT>
Error writeOutputFile(const Options &Options, ELFObjectFile<ELFT> &InputFile,
                      DataBits &OutBits) {
  DwarfutilConfigManager Config;

  Config.InputFilename = Options.InputFileName;
  Config.OutputFilename = Options.OutputFileName;
  if (Options.Compress)
    Config.CompressionType = DebugCompressionType::GNU;
  Config.DecompressDebugSections = Options.Decompress;

  if (Options.StripAll) {
    Config.StripDebug = true;
  } else if (Options.Decompress) {
    Config.DecompressDebugSections = true;
  } else {
    if (Expected<ELFObjectFile<ELFT>> MemFile =
            ELFObjectFile<ELFT>::create(MemoryBufferRef(OutBits, ""))) {
      if (Options.StripUnoptimizedDebug) {
        // Delete all debug sections. Debug sections which have optimized
        // pair should be deleted as they would be replaced with paired
        // optimized version. Debug section which does not have optimized pair
        // should be deleted since they could become incorrect after debug
        // info has been optimized.
        Error Err =
            Config.ToRemove.addMatcher(objectcopy::NameOrPattern::create(
                "\\.debug*", objectcopy::MatchStyle::Wildcard,
                [](Error Err) -> Error { return Err; }));
        error(ToolName, std::move(Err));

        Err = Config.ToRemove.addMatcher(objectcopy::NameOrPattern::create(
            "\\.gdb_index", objectcopy::MatchStyle::Wildcard,
            [](Error Err) -> Error { return Err; }));
        error(ToolName, std::move(Err));
      } else {
        // Delete debug sections which have optimized pair as they should be
        // replaced with paired optimized version.
        for (const auto &Section : MemFile->sections()) {
          if (Expected<StringRef> SectionName = Section.getName()) {
            if (SectionName->startswith(".debug") ||
                *SectionName == ".gdb_index") {
              Error Err =
                  Config.ToRemove.addMatcher(objectcopy::NameOrPattern::create(
                      *SectionName, objectcopy::MatchStyle::Wildcard,
                      [](Error Err) -> Error { return Err; }));
              error(ToolName, std::move(Err));
            }
          }
        }
      }

      // Add new debug sections.
      for (const SectionRef &Sec : MemFile->sections()) {
        if (Expected<StringRef> SecName = Sec.getName())
          if (SecName->startswith(".debug") || *SecName == ".gdb_index") {
            if (Expected<StringRef> SecData = Sec.getContents()) {
              Config.NewDebugSections.insert(
                  std::make_pair(*SecName, *SecData));
            }
          }
      }
    } else {
      error(ToolName, createStringError(std::errc::invalid_argument,
                                        "unknown command line option."));
    }
  }

  objectcopy::FileBuffer FB(Config.OutputFilename);
  return objectcopy::elf::executeObjcopyOnBinary(Config.getCommonConfig(), Config.getElfConfig(), InputFile, FB);  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
}

With the design suggested by the current(D99055) patch there would be created local DwarfutilConfigManager which implements MultiFormatConfig interface. The objectcopy::elf::executeObjcopyOnBinary function from object library would get the options by call to getCommonConfig(), getElfConfig(). To have access to objectcopy functions the https://reviews.llvm.org/D88827 is created. The goal of https://reviews.llvm.org/D88827 is to move object copy functions from the specific tool to the object library area. So that object copy functions would be available to the alternative usages(not only from llvm-objcopy tool):

Error executeObjcopyOnIHex(const CommonConfig &CommonCfg, const ElfConfig &ElfCfg, MemoryBuffer &In,
                           raw_ostream &Out);
Error executeObjcopyOnRawBinary(const CommonConfig &CommonCfg, const ElfConfig &ElfCfg, MemoryBuffer &In,
                                raw_ostream &Out);
Error executeObjcopyOnBinary(const CommonConfig &CommonCfg, const ElfConfig &ElfCfg,
                             object::ELFObjectFileBase &In, raw_ostream &Out);

Error executeObjcopyOnBinary(const CommonConfig &CommonCfg, const COFFConfig &COFFCfg,
                             object::COFFObjectFile &In, raw_ostream &Out);

Error executeObjcopyOnBinary(const CommonConfig &CommonCfg, const MachOConfig &MachOCfg,
                             object::MachOObjectFile &In, raw_ostream &Out);
Error executeObjcopyOnMachOUniversalBinary(
    const MultiFormatConfig &Config, const object::MachOUniversalBinary &In, raw_ostream &Out);

Error executeObjcopyOnBinary(const CommonConfig &CommonCfg, const MachOConfig &MachOCfg,
                             object::WasmObjectFile &In, raw_ostream &Out);

Error executeObjcopyOnArchive(const MultiFormatConfig &Config, const object::Archive &Ar)

[avl]

ping.

[dblaikie]

llvm/tools/llvm-objcopy/ConfigManager.h

27

I'm not sure how generic the common part is if it has to call for/have specifically enumerated options for each specific part, though? I'm not sure how much value that adds.

But also I realize this code review seems like it's gone on/been through so many different fairly expensive variations, that I don't want to come in half-informed and complicate this review further. I'd like to try to simplify/streamline the review a bit if I can help with tie-breakers or other things that might enable progress.

@jhenderson (@rupprecht?) - is there some way we could help settle this work a bit more than it seems to have been able to be so far? (happy to video chat, etc, if that might be helpful)

[avl]

llvm/tools/llvm-objcopy/ConfigManager.h

27

I'm not sure how generic the common part is if it has to call for/have specifically enumerated options for each specific part, though? I'm not sure how much value that adds.

The necessity to call for the specific part is driven by the fact that there exist options that could not be parsed without knowing the final format. That is the reason why parsing such options is delayed until the final format is known.

Probably, instead of delayed parsing, we might detect the final format by pre-check. i.e. before starting to parse options we inspect the files and detect the format. Then we would be able to parse options properly and create a fully parsed CopyConfig from the scratch. No extra call for specific options would be required.

In that case CopyConfig might be single structure containing all options(without lazy initialized parts).

[dblaikie]

llvm/tools/llvm-objcopy/ConfigManager.h

27

The necessity to call for the specific part is driven by the fact that there exist options that could not be parsed without knowing the final format. That is the reason why parsing such options is delayed until the final format is known.

*nod* Though the current code doesn't have this abstraction and seems to get along OK, yeah? It does that by having some options be implicitly invalid until some operation is called that parses and populations those options (based on which file format is in use)?

[jhenderson]

But also I realize this code review seems like it's gone on/been through so many different fairly expensive variations, that I don't want to come in half-informed and complicate this review further. I'd like to try to simplify/streamline the review a bit if I can help with tie-breakers or other things that might enable progress.

@jhenderson (@rupprecht?) - is there some way we could help settle this work a bit more than it seems to have been able to be so far? (happy to video chat, etc, if that might be helpful)

Sorry, I've been snowed under with other reviews, combined with workloads elsewhere, so I haven't been able to keep focused on this patch and given it the time it needs. It's also meant that as I've come back to it every so often, I see things differently, hence the occasional changes in opinion.

FWIW, I think the current design generally looks reasonably good to me, but I'm open to other design ideas that satisfy the goals of both the new use-case and the existing behaviour. If an alternative looks simpler/more effective, please feel free to shout, though it might be easier to write it in a different patch so that we can compare and contrast competing approaches.

@avl, when I meant see a use-case, I meant literally see a patch that makes use of this refactoring in its current form. If a later patch can't be cleanly based on this patch, then it indicates a problem with the current design.

Aside: the big if (/*various option checks*/ for the file formats is always something that's bothered me, and I wonder if we really need them. Perhaps that's a different conversation to be had, but it feels like not having them would somewhat simplify some of this code too.

llvm/tools/llvm-objcopy/ConfigManager.cpp

709

I specifically meant the name Config in my example (well, with a typo...), as it stops you needing to constantly change Config to CommonCfg in this function. The same sort of thing goes throughout this patch. In other words, Config would represent the common stuff, and then ELFConfig etc would represent the format-specific stuff.

1060–1061

Same here as above - use Config to refer to the CommonConfig to save having to rename everything. Same goes thoughout this patch.

[avl]

FWIW, I think the current design generally looks reasonably good to me, but I'm open to other design ideas that satisfy the goals of both the new use-case and the existing behaviour. If an alternative looks simpler/more effective, please feel free to shout, though it might be easier to write it in a different patch so that we can compare and contrast competing approaches.

The current design is also OK from my point of view. The only thing is that it seems to me that using inheritance instead of encapsulation for CopyConfig structures would make usage more convenient/simple(https://reviews.llvm.org/D99055?id=334944).

@avl, when I meant see a use-case, I meant literally see a patch that makes use of this refactoring in its current form. If a later patch can't be cleanly based on this patch, then it indicates a problem with the current design.

It would take a time to prepare such a patch. The fully ready use case - is some fully implemented tool, it requires a lot of other refactorings and changes. Anyway, would change https://reviews.llvm.org/D86539 to use refactoring from this patch.

llvm/tools/llvm-objcopy/ConfigManager.cpp

709

OK.

[avl]

llvm/tools/llvm-objcopy/ConfigManager.h

27

Right. This - "having some options be implicitly invalid until some operation is called that parses and populations those options" - makes it bad for general interface. Because the interface contains extra and partially incomplete options. "MultiFormatConfig" abstraction hides these details. Thus, unparsed and unintialized options are not seen inside createNewArchiveMembers. In other words, implementation details of options processing of llvm-objcopy are hidden behind MultiFormatConfig interface. This looks good for general interface.

[dblaikie]

In D99055#2747391, @jhenderson wrote:

...
FWIW, I think the current design generally looks reasonably good to me,

Nah, that's god enough for me - wanted to help ensure this review was converging, that's all. I'll step back.

[avl]

renamed as requested.

[jhenderson]

I've done another skim. I should be able to give this a fuller review again tomorrow.

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

1

Revert the "C++" bit of this change. The old-style was correct - see https://llvm.org/docs/CodingStandards.html#file-headers. In fact, looking at the other headers in this change, please add this to them too.

Also "ELFConfig.h" not "ElfConfig.h" is more correct, as "ELF" is an accronym, so should be all-caps in general.

29–31

Similar comment: "ELF specific" and ELFConfig are the correct spellings.

llvm/tools/llvm-objcopy/MachOConfig.h

15 ↗

(On Diff #344350)

llvm/tools/llvm-objcopy/MultiFormatConfig.h

13–15

vector and Allocator.h look unnecessary to me.

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

12–14

Is there a particular reason you haven't put these files inside the COFF/ELF/MachO/wasm subdirectories?

[avl]

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

12–14

They all supposed to go away from the llvm/tools/llvm-objcopy directory. They would be put into the include/llvm/Object/ObjCopy? directory later. So I put it together for now. If it is necessary we might temporarily place them into COFF/ELF/MachO/wasm subdirectories until library review is not ready.

[jhenderson]

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

12–14

I don't think it's particularly hard to move these headers once the code is pulled into a library at a later date. Given that there will be a presumed interim period when this patch will have landed, but the library does not exist yet, we should have everything make sense in the interim. That would imply the file format specific files should be in their corresponding subdirectories. After all, the files that are already in those directories will need to move too, right?

[avl]

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

12–14

yes, that`s right. will put above into the specific directories.

[avl]

addressed comments.

[jhenderson]

Do you actually need WasmConfig and COFFConfig? If we can avoid adding them, that would be nice.

llvm/tools/llvm-objcopy/COFF/COFFConfig.h

10

Need to fix the include guards. Same in the other headers, as directed by clang-tidy.

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

1

Still need to change ElfConfig.h to ELFConfig.h in the comment on the first line of the file.

llvm/tools/llvm-objcopy/MultiFormatConfig.h

2

Nit: missing the "C++" bit in this line of the comment.

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

273–274

Perhaps add a CommonConfig &Config local variable to this function? Would allow you to avoid repeated getCommonConfig() calls.

326

Similar to elsewhere, consider adding a CommonConfig &Config local variable to reduce the repetitive ConfigMgr.Common statements, and reduce changes in this diff.

[avl]

Do you actually need WasmConfig and COFFConfig? If we can avoid adding them, that would be nice.

Other than specific options set we also need to do following calls for validation:

virtual Expected<const COFFConfig &> getCOFFConfig() const = 0;
virtual Expected<const WasmConfig &> getWasmConfig() const = 0;

I think it is good to have these calls to be done in the same way as other getSpecificConfig() methods. Thus, It looks like we still need to have WasmConfig and COFFConfig structures.

[avl]

addressed comments.

[jhenderson]

Okay, I think we're there, LGTM! Might want to give it a couple of days before landing, in case others have any comments.

[avl]

Thank you for the review.

llvm/tools/llvm-objcopy/CMakeLists.txt

 tablegen(LLVM BitcodeStripOpts.inc -gen-opt-parser-defs)
 add_public_tablegen_target(BitcodeStripOptsTableGen)
 
 set(LLVM_TARGET_DEFINITIONS StripOpts.td)
 tablegen(LLVM StripOpts.inc -gen-opt-parser-defs)
 add_public_tablegen_target(StripOptsTableGen)
 
 add_llvm_tool(llvm-objcopy
-  CopyConfig.cpp
+  ConfigManager.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
   wasm/Object.cpp

llvm/tools/llvm-objcopy/COFF/COFFConfig.h

+//===- COFFConfig.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_LLVM_OBJCOPY_COFF_COFFCONFIG_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_COFF_COFFCONFIG_H
+
+namespace llvm {
+namespace objcopy {
+
+// Coff specific configuration for copying/stripping a single file.
+struct COFFConfig {};
+
+} // namespace objcopy
+} // namespace llvm
+
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_COFF_COFFCONFIG_H

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.h

 class Error;
 class raw_ostream;
 
 namespace object {
 class COFFObjectFile;
 } // end namespace object
 
 namespace objcopy {
-struct CopyConfig;
+struct CommonConfig;
+struct COFFConfig;
 
 namespace coff {
-Error executeObjcopyOnBinary(const CopyConfig &Config,
+
+Error executeObjcopyOnBinary(const CommonConfig &Config, const COFFConfig &,
                              object::COFFObjectFile &In, raw_ostream &Out);
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFFOBJCOPY_H

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 //===- COFFObjcopy.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 "COFFObjcopy.h"
-#include "CopyConfig.h"
+#include "COFFConfig.h"
+#include "CommonConfig.h"
 #include "Object.h"
 #include "Reader.h"
 #include "Writer.h"
 
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Support/CRC.h"
 #include "llvm/Support/Errc.h"
static void setSectionFlags(Section &Sec, SectionFlag AllFlags) {
   if (AllFlags & SectionFlag::SecShare)
     NewCharacteristics |= IMAGE_SCN_MEM_SHARED;
   if (AllFlags & SectionFlag::SecExclude)
     NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
 
   Sec.Header.Characteristics = NewCharacteristics;
 }
 
-static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
     // Contrary to --only-keep-debug, --only-section fully removes sections that
     // aren't mentioned.
     if (!Config.OnlySection.empty() && !Config.OnlySection.matches(Sec.Name))
       return true;
 
     if (Config.StripDebug || Config.StripAll || Config.StripAllGNU ||
addSection(
                      Buf->getBufferSize()),
         IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES);
   }
 
   if (!Config.AddGnuDebugLink.empty())
     if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
       return E;
 
-  if (Config.AllowBrokenLinks || !Config.SplitDWO.empty() ||
-      !Config.SymbolsPrefix.empty() || !Config.AllocSectionsPrefix.empty() ||
-      !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
-      Config.NewSymbolVisibility || !Config.SymbolsToGlobalize.empty() ||
-      !Config.SymbolsToKeep.empty() || !Config.SymbolsToLocalize.empty() ||
-      !Config.SymbolsToWeaken.empty() || !Config.SymbolsToKeepGlobal.empty() ||
-      !Config.SectionsToRename.empty() || !Config.SetSectionAlignment.empty() ||
-      Config.ExtractDWO || Config.LocalizeHidden || Config.PreserveDates ||
-      Config.StripDWO || Config.StripNonAlloc || Config.StripSections ||
-      Config.StripSwiftSymbols || Config.KeepUndefined || Config.Weaken ||
-      Config.DecompressDebugSections ||
-      Config.DiscardMode == DiscardType::Locals ||
-      !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
-    return createStringError(llvm::errc::invalid_argument,
-                             "option not supported by llvm-objcopy for COFF");
-  }
-
   return Error::success();
 }
 
-Error executeObjcopyOnBinary(const CopyConfig &Config, COFFObjectFile &In,
-                             raw_ostream &Out) {
+Error executeObjcopyOnBinary(const CommonConfig &Config, const COFFConfig &,
+                             COFFObjectFile &In, raw_ostream &Out) {
   COFFReader Reader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
   if (!ObjOrErr)
     return createFileError(Config.InputFilename, ObjOrErr.takeError());
   Object *Obj = ObjOrErr->get();
   assert(Obj && "Unable to deserialize COFF object");
   if (Error E = handleArgs(Config, *Obj))
     return createFileError(Config.InputFilename, std::move(E));

llvm/tools/llvm-objcopy/CommonConfig.h

-//===- CopyConfig.h -------------------------------------------------------===//
+//===- CommonConfig.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_LLVM_OBJCOPY_COPY_CONFIG_H
-#define LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
+#ifndef LLVM_TOOLS_LLVM_OBJCOPY_COMMONCONFIG_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_COMMONCONFIG_H
 
-#include "ELF/ELFConfig.h"
 #include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/DenseSet.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"
-#include "llvm/Support/Error.h"
 #include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/Regex.h"
 // Necessary for llvm::DebugCompressionType::None
 #include "llvm/Target/TargetOptions.h"
 #include <vector>
 
 namespace llvm {
 namespace objcopy {

 enum class MatchStyle {
   Literal,  // Default for symbols.
   Wildcard, // Default for sections, or enabled with --wildcard (-w).
   Regex,    // Enabled with --regex.
 };
 
 class NameOrPattern {
   StringRef Name;
-  // Regex is shared between multiple CopyConfig instances.
+  // Regex is shared between multiple CommonConfig instances.
   std::shared_ptr<Regex> R;
   std::shared_ptr<GlobPattern> G;
   bool IsPositiveMatch = true;
 
   NameOrPattern(StringRef N) : Name(N) {}
   NameOrPattern(std::shared_ptr<Regex> R) : R(R) {}
   NameOrPattern(std::shared_ptr<GlobPattern> G, bool IsPositiveMatch)
       : G(G), IsPositiveMatch(IsPositiveMatch) {}
public:
   }
   bool matches(StringRef S) const {
     return is_contained(PosMatchers, S) && !is_contained(NegMatchers, S);
   }
   bool empty() const { return PosMatchers.empty() && NegMatchers.empty(); }
 };
 
 // Configuration for copying/stripping a single file.
-struct CopyConfig {
-  // Format-specific options to be initialized lazily when needed.
-  Optional<elf::ELFCopyConfig> ELF;
-
+struct CommonConfig {
   // Main input/output options
   StringRef InputFilename;
   FileFormat InputFormat = FileFormat::Unspecified;
   StringRef OutputFilename;
   FileFormat OutputFormat = FileFormat::Unspecified;
 
   // 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;
   Optional<StringRef> ExtractPartition;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
   StringRef AllocSectionsPrefix;
   DiscardType DiscardMode = DiscardType::None;
-  Optional<StringRef> NewSymbolVisibility;
 
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
-  std::vector<StringRef> SymbolsToAdd;
   std::vector<StringRef> RPathToAdd;
   std::vector<StringRef> RPathToPrepend;
   DenseMap<StringRef, StringRef> RPathsToUpdate;
   DenseMap<StringRef, StringRef> InstallNamesToUpdate;
   DenseSet<StringRef> RPathsToRemove;
 
   // install-name-tool's id option
   Optional<StringRef> SharedLibId;
struct CommonConfig {
   bool StripSwiftSymbols = false;
   bool StripUnneeded = false;
   bool Weaken = false;
   bool DecompressDebugSections = false;
   // install-name-tool's --delete_all_rpaths
   bool RemoveAllRpaths = 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;
 };
 
-// ParseObjcopyOptions returns the config and sets the input arguments. If a
-// help flag is set then ParseObjcopyOptions will print the help messege and
-// exit. ErrorCallback is used to handle recoverable errors. An Error returned
-// by the callback aborts the parsing and is then returned by this function.
-Expected<DriverConfig>
-parseObjcopyOptions(ArrayRef<const char *> ArgsArr,
-                    llvm::function_ref<Error(Error)> ErrorCallback);
-
-// ParseInstallNameToolOptions returns the config and sets the input arguments.
-// If a help flag is set then ParseInstallNameToolOptions will print the help
-// messege and exit.
-Expected<DriverConfig>
-parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr);
-
-// ParseBitcodeStripOptions returns the config and sets the input arguments.
-// If a help flag is set then ParseBitcodeStripOptions will print the help
-// messege and exit.
-Expected<DriverConfig> parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr);
-
-// ParseStripOptions returns the config and sets the input arguments. If a
-// help flag is set then ParseStripOptions will print the help messege and
-// exit. ErrorCallback is used to handle recoverable errors. An Error returned
-// by the callback aborts the parsing and is then returned by this function.
-Expected<DriverConfig>
-parseStripOptions(ArrayRef<const char *> ArgsArr,
-                  llvm::function_ref<Error(Error)> ErrorCallback);
 } // namespace objcopy
 } // namespace llvm
 
-#endif
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_COMMONCONFIG_H

llvm/tools/llvm-objcopy/ConfigManager.h

+//===- ConfigManager.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_CONFIGMANAGER_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_CONFIGMANAGER_H
+
+#include "COFF/COFFConfig.h"
+#include "CommonConfig.h"
+#include "ELF/ELFConfig.h"
+#include "MachO/MachOConfig.h"
+#include "MultiFormatConfig.h"
+#include "wasm/WasmConfig.h"
+#include "llvm/Support/Allocator.h"
+#include <vector>
+
+namespace llvm {
+namespace objcopy {
+
+// ConfigManager keeps all configurations and prepare
+// format-specific options.
+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;
+  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.
+struct DriverConfig {
+  SmallVector<ConfigManager, 1> CopyConfigs;
+  BumpPtrAllocator Alloc;
+};
+
+// ParseObjcopyOptions returns the config and sets the input arguments. If a
+// help flag is set then ParseObjcopyOptions will print the help messege and
+// exit. ErrorCallback is used to handle recoverable errors. An Error returned
+// by the callback aborts the parsing and is then returned by this function.
+Expected<DriverConfig>
+parseObjcopyOptions(ArrayRef<const char *> ArgsArr,
+                    llvm::function_ref<Error(Error)> ErrorCallback);
+
+// ParseInstallNameToolOptions returns the config and sets the input arguments.
+// If a help flag is set then ParseInstallNameToolOptions will print the help
+// messege and exit.
+Expected<DriverConfig>
+parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr);
+
+// ParseBitcodeStripOptions returns the config and sets the input arguments.
+// If a help flag is set then ParseBitcodeStripOptions will print the help
+// messege and exit.
+Expected<DriverConfig> parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr);
+
+// ParseStripOptions returns the config and sets the input arguments. If a
+// help flag is set then ParseStripOptions will print the help messege and
+// exit. ErrorCallback is used to handle recoverable errors. An Error returned
+// by the callback aborts the parsing and is then returned by this function.
+Expected<DriverConfig>
+parseStripOptions(ArrayRef<const char *> ArgsArr,
+                  llvm::function_ref<Error(Error)> ErrorCallback);
+} // namespace objcopy
+} // namespace llvm
+
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_CONFIGMANAGER_H

llvm/tools/llvm-objcopy/ConfigManager.cpp

-//===- CopyConfig.cpp -----------------------------------------------------===//
+//===- ConfigManager.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 "ConfigManager.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CRC.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
 namespace {
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) {
+  // 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<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;
+}
+
+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() ||
+      !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) {
+    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.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) {
+    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.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(
+        llvm::errc::invalid_argument,
+        "only add-section, dump-section, and remove-section are supported");
+  }
+
+  return Wasm;
+}
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig>
 parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                     llvm::function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ObjcopyOptTable T;
parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
 
   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");
 
-  CopyConfig Config;
+  ConfigManager ConfigMgr;
+  CommonConfig &Config = ConfigMgr.Common;
   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");
 
   if (InputArgs.hasArg(OBJCOPY_regex) && InputArgs.hasArg(OBJCOPY_wildcard))
parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
   // 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))
-    Config.NewSymbolVisibility =
+    ConfigMgr.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()) {
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))
-    Config.SymbolsToAdd.push_back(Arg->getValue());
+    ConfigMgr.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);
return createStringError(
         errc::invalid_argument,
         "LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress");
 
   if (Config.ExtractPartition && Config.ExtractMainPartition)
     return createStringError(errc::invalid_argument,
                              "cannot specify --extract-partition together with "
                              "--extract-main-partition");
 
-  DC.CopyConfigs.push_back(std::move(Config));
+  DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }
 
 // ParseInstallNameToolOptions returns the config and sets the input arguments.
 // If a help flag is set then ParseInstallNameToolOptions will print the help
 // messege and exit.
 Expected<DriverConfig>
 parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
-  CopyConfig Config;
+  ConfigManager ConfigMgr;
+  CommonConfig &Config = ConfigMgr.Common;
   InstallNameToolOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (MissingArgumentCount)
     return createStringError(
         errc::invalid_argument,
if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
   if (Positional.size() > 1)
     return createStringError(
         errc::invalid_argument,
         "llvm-install-name-tool expects a single input file");
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[0];
 
-  DC.CopyConfigs.push_back(std::move(Config));
+  DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }
 
 Expected<DriverConfig>
 parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
-  CopyConfig Config;
+  ConfigManager ConfigMgr;
+  CommonConfig &Config = ConfigMgr.Common;
   BitcodeStripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     printHelp(T, errs(), ToolType::BitcodeStrip);
     exit(1);
for (auto *Arg : InputArgs.filtered(BITCODE_STRIP_INPUT))
     Positional.push_back(Arg->getValue());
   if (Positional.size() > 1)
     return createStringError(errc::invalid_argument,
                              "llvm-bitcode-strip expects a single input file");
   assert(!Positional.empty());
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[0];
 
-  DC.CopyConfigs.push_back(std::move(Config));
+  DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
 Expected<DriverConfig>
 parseStripOptions(ArrayRef<const char *> ArgsArr,
parseStripOptions(ArrayRef<const char *> ArgsArr,
   if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output))
     return createStringError(
         errc::invalid_argument,
         "multiple input files cannot be used in combination with -o");
 
-  CopyConfig Config;
+  ConfigManager ConfigMgr;
+  CommonConfig &Config = ConfigMgr.Common;
 
   if (InputArgs.hasArg(STRIP_regex) && InputArgs.hasArg(STRIP_wildcard))
     return createStringError(errc::invalid_argument,
                              "--regex and --wildcard are incompatible");
   MatchStyle SectionMatchStyle =
       InputArgs.hasArg(STRIP_regex) ? MatchStyle::Regex : MatchStyle::Wildcard;
   MatchStyle SymbolMatchStyle = InputArgs.hasArg(STRIP_regex)
                                     ? MatchStyle::Regex
parseStripOptions(ArrayRef<const char *> ArgsArr,
   Config.InputFormat = FileFormat::Unspecified;
   Config.OutputFormat = FileFormat::Unspecified;
 
   DriverConfig DC;
   if (Positional.size() == 1) {
     Config.InputFilename = Positional[0];
     Config.OutputFilename =
         InputArgs.getLastArgValue(STRIP_output, Positional[0]);
-    DC.CopyConfigs.push_back(std::move(Config));
+    DC.CopyConfigs.push_back(std::move(ConfigMgr));
   } else {
     StringMap<unsigned> InputFiles;
     for (StringRef Filename : Positional) {
       if (InputFiles[Filename]++ == 1) {
         if (Filename == "-")
           return createStringError(
               errc::invalid_argument,
               "cannot specify '-' as an input file more than once");
         if (Error E = ErrorCallback(createStringError(
                 errc::invalid_argument, "'%s' was already specified",
                 Filename.str().c_str())))
           return std::move(E);
       }
       Config.InputFilename = Filename;
       Config.OutputFilename = Filename;
-      DC.CopyConfigs.push_back(Config);
+      DC.CopyConfigs.push_back(ConfigMgr);
     }
   }
 
   if (Config.PreserveDates && (is_contained(Positional, "-") ||
                                InputArgs.getLastArgValue(STRIP_output) == "-"))
     return createStringError(errc::invalid_argument,
                              "--preserve-dates requires a file");
 
   return std::move(DC);
 }
 
 } // namespace objcopy
 } // namespace llvm

llvm/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
+#ifndef LLVM_TOOLS_LLVM_OBJCOPY_ELF_ELFCONFIG_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_ELF_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 {
+// ELF specific configuration for copying/stripping a single file.
+struct ELFConfig {
   Optional<uint8_t> NewSymbolVisibility;
   std::vector<NewSymbolInfo> SymbolsToAdd;
 };
 
-Expected<ELFCopyConfig> parseConfig(const CopyConfig &Config);
-
-} // namespace elf
 } // namespace objcopy
 } // namespace llvm
 
-#endif
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_ELF_ELFCONFIG_H

llvm/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/tools/llvm-objcopy/ELF/ELFObjcopy.h

 class MemoryBuffer;
 class raw_ostream;
 
 namespace object {
 class ELFObjectFileBase;
 } // end namespace object
 
 namespace objcopy {
-struct CopyConfig;
+struct CommonConfig;
+struct ELFConfig;
 
 namespace elf {
-Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnIHex(const CommonConfig &Config,
+                           const ELFConfig &ELFConfig, MemoryBuffer &In,
                            raw_ostream &Out);
-Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnRawBinary(const CommonConfig &Config,
+                                const ELFConfig &ELFConfig, MemoryBuffer &In,
                                 raw_ostream &Out);
-Error executeObjcopyOnBinary(const CopyConfig &Config,
+Error executeObjcopyOnBinary(const CommonConfig &Config,
+                             const ELFConfig &ELFConfig,
                              object::ELFObjectFileBase &In, raw_ostream &Out);
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_ELFOBJCOPY_H

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

 //===- ELFObjcopy.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 "ELFObjcopy.h"
-#include "CopyConfig.h"
+#include "CommonConfig.h"
+#include "ELFConfig.h"
 #include "Object.h"
 #include "llvm-objcopy.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"

 static ElfType getOutputElfType(const MachineInfo &MI) {
   // Infer output ELF type from the binary arch specified
   if (MI.Is64Bit)
     return MI.IsLittleEndian ? ELFT_ELF64LE : ELFT_ELF64BE;
   else
     return MI.IsLittleEndian ? ELFT_ELF32LE : ELFT_ELF32BE;
 }
 
-static std::unique_ptr<Writer> createELFWriter(const CopyConfig &Config,
+static std::unique_ptr<Writer> createELFWriter(const CommonConfig &Config,
                                                Object &Obj, raw_ostream &Out,
                                                ElfType OutputElfType) {
   // Depending on the initial ELFT and OutputFormat we need a different Writer.
   switch (OutputElfType) {
   case ELFT_ELF32LE:
     return std::make_unique<ELFWriter<ELF32LE>>(Obj, Out, !Config.StripSections,
                                                 Config.OnlyKeepDebug);
   case ELFT_ELF64LE:
     return std::make_unique<ELFWriter<ELF64LE>>(Obj, Out, !Config.StripSections,
                                                 Config.OnlyKeepDebug);
   case ELFT_ELF32BE:
     return std::make_unique<ELFWriter<ELF32BE>>(Obj, Out, !Config.StripSections,
                                                 Config.OnlyKeepDebug);
   case ELFT_ELF64BE:
     return std::make_unique<ELFWriter<ELF64BE>>(Obj, Out, !Config.StripSections,
                                                 Config.OnlyKeepDebug);
   }
   llvm_unreachable("Invalid output format");
 }
 
-static std::unique_ptr<Writer> createWriter(const CopyConfig &Config,
+static std::unique_ptr<Writer> createWriter(const CommonConfig &Config,
                                             Object &Obj, raw_ostream &Out,
                                             ElfType OutputElfType) {
   switch (Config.OutputFormat) {
   case FileFormat::Binary:
     return std::make_unique<BinaryWriter>(Obj, Out);
   case FileFormat::IHex:
     return std::make_unique<IHexWriter>(Obj, Out);
   default:

 }
 
 static bool isUnneededSymbol(const Symbol &Sym) {
   return !Sym.Referenced &&
          (Sym.Binding == STB_LOCAL || Sym.getShndx() == SHN_UNDEF) &&
          Sym.Type != STT_SECTION;
 }
 
-static Error updateAndRemoveSymbols(const CopyConfig &Config, Object &Obj) {
+static Error updateAndRemoveSymbols(const CommonConfig &Config, Object &Obj) {
   // TODO: update or remove symbols only if there is an option that affects
   // them.
   if (!Obj.SymbolTable)
     return Error::success();
 
   Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
     // Common and undefined symbols don't make sense as local symbols, and can
     // even cause crashes if we localize those, so skip them.
if (!Config.OnlySection.empty() && !Sym.Referenced &&
       return true;
 
     return false;
   };
 
   return Obj.removeSymbols(RemoveSymbolsPred);
 }
 
-static Error replaceAndRemoveSections(const CopyConfig &Config, Object &Obj) {
+static Error replaceAndRemoveSections(const CommonConfig &Config, Object &Obj) {
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const SectionBase &Sec) {
       return Config.ToRemove.matches(Sec.Name);
     };
   }

 
 // 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 CopyConfig &Config, Object &Obj) {
-  if (Config.StripSwiftSymbols || Config.KeepUndefined)
-    return createStringError(llvm::errc::invalid_argument,
-                             "option not supported by llvm-objcopy for ELF");
-
+static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
+                        Object &Obj) {
   if (Config.OutputArch) {
     Obj.Machine = Config.OutputArch.getValue().EMachine;
     Obj.OSABI = Config.OutputArch.getValue().OSABI;
   }
 
   if (!Config.SplitDWO.empty() && Config.ExtractDWO) {
     return Obj.removeSections(
         Config.AllowBrokenLinks,
         [&Obj](const SectionBase &Sec) { return onlyKeepDWOPred(Obj, Sec); });
   }
 
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 && !Config.ELF->SymbolsToAdd.empty())
+  if (!Obj.SymbolTable && !ELFConfig.SymbolsToAdd.empty())
     if (Error E = Obj.addNewSymbolTable())
       return E;
 
-  for (const NewSymbolInfo &SI : Config.ELF->SymbolsToAdd) {
+  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);
   }
 
   // --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);
       }
     }
   }
 
   if (Config.EntryExpr)
     Obj.Entry = Config.EntryExpr(Obj.Entry);
   return Error::success();
 }
 
-static Error writeOutput(const CopyConfig &Config, Object &Obj,
+static Error writeOutput(const CommonConfig &Config, Object &Obj,
                          raw_ostream &Out, ElfType OutputElfType) {
   std::unique_ptr<Writer> Writer =
       createWriter(Config, Obj, Out, OutputElfType);
   if (Error E = Writer->finalize())
     return E;
   return Writer->write();
 }
 
-Error executeObjcopyOnIHex(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnIHex(const CommonConfig &Config,
+                           const ELFConfig &ELFConfig, MemoryBuffer &In,
                            raw_ostream &Out) {
   IHexReader Reader(&In);
   Expected<std::unique_ptr<Object>> Obj = Reader.create(true);
   if (!Obj)
     return Obj.takeError();
 
   const ElfType OutputElfType =
       getOutputElfType(Config.OutputArch.getValueOr(MachineInfo()));
-  if (Error E = handleArgs(Config, **Obj))
+  if (Error E = handleArgs(Config, ELFConfig, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
-Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnRawBinary(const CommonConfig &Config,
+                                const ELFConfig &ELFConfig, MemoryBuffer &In,
                                 raw_ostream &Out) {
   uint8_t NewSymbolVisibility =
-      Config.ELF->NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
+      ELFConfig.NewSymbolVisibility.getValueOr((uint8_t)ELF::STV_DEFAULT);
   BinaryReader Reader(&In, 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, **Obj))
+  if (Error E = handleArgs(Config, ELFConfig, **Obj))
     return E;
   return writeOutput(Config, **Obj, Out, OutputElfType);
 }
 
-Error executeObjcopyOnBinary(const CopyConfig &Config,
+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(!Config.SymbolsToAdd.empty());
+      Reader.create(!ELFConfig.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, **Obj))
+  if (Error E = handleArgs(Config, ELFConfig, **Obj))
     return createFileError(Config.InputFilename, std::move(E));
 
   if (Error E = writeOutput(Config, **Obj, Out, OutputElfType))
     return createFileError(Config.InputFilename, std::move(E));
 
   return Error::success();
 }
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm

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

 //===- Object.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_OBJECT_H
 #define LLVM_TOOLS_OBJCOPY_OBJECT_H
 
-#include "CopyConfig.h"
+#include "CommonConfig.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileOutputBuffer.h"

llvm/tools/llvm-objcopy/MachO/MachOConfig.h

+//===- MachOConfig.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_LLVM_OBJCOPY_MACHO_MACHOCONFIG_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_MACHO_MACHOCONFIG_H
+
+namespace llvm {
+namespace objcopy {
+
+// Mach-O specific configuration for copying/stripping a single file.
+struct MachOConfig {};
+
+} // namespace objcopy
+} // namespace llvm
+
+#endif // LLVM_TOOLS_LLVM_OBJCOPY_MACHO_MACHOCONFIG_H

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.h

 class raw_ostream;
 
 namespace object {
 class MachOObjectFile;
 class MachOUniversalBinary;
 } // end namespace object
 
 namespace objcopy {
-struct CopyConfig;
-class Buffer;
+struct CommonConfig;
+struct MachOConfig;
+class MultiFormatConfig;
 
 namespace macho {
-Error executeObjcopyOnBinary(const CopyConfig &Config,
+Error executeObjcopyOnBinary(const CommonConfig &Config, const MachOConfig &,
                              object::MachOObjectFile &In, raw_ostream &Out);
 
 Error executeObjcopyOnMachOUniversalBinary(
-    CopyConfig &Config, const object::MachOUniversalBinary &In,
+    const MultiFormatConfig &Config, const object::MachOUniversalBinary &In,
     raw_ostream &Out);
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_MACHOOBJCOPY_H

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

 //===- MachOObjcopy.cpp -----------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "MachOObjcopy.h"
-#include "../CopyConfig.h"
 #include "../llvm-objcopy.h"
+#include "CommonConfig.h"
 #include "MachOReader.h"
 #include "MachOWriter.h"
+#include "MultiFormatConfig.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/MachOUniversalWriter.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/SmallVectorMemoryBuffer.h"
static StringRef getPayloadString(const LoadCommand &LC) {
   assert(isLoadCommandWithPayloadString(LC) &&
          "unsupported load command encountered");
 
   return StringRef(reinterpret_cast<const char *>(LC.Payload.data()),
                    LC.Payload.size())
       .rtrim('\0');
 }
 
-static Error removeSections(const CopyConfig &Config, Object &Obj) {
+static Error removeSections(const CommonConfig &Config, Object &Obj) {
   SectionPred RemovePred = [](const std::unique_ptr<Section> &) {
     return false;
   };
 
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config, RemovePred](const std::unique_ptr<Section> &Sec) {
       return Config.ToRemove.matches(Sec->CanonicalName);
     };
if (!Config.OnlySection.empty()) {
     RemovePred = [&Config](const std::unique_ptr<Section> &Sec) {
       return !Config.OnlySection.matches(Sec->CanonicalName);
     };
   }
 
   return Obj.removeSections(RemovePred);
 }
 
-static void markSymbols(const CopyConfig &Config, Object &Obj) {
+static void markSymbols(const CommonConfig &, Object &Obj) {
   // Symbols referenced from the indirect symbol table must not be removed.
   for (IndirectSymbolEntry &ISE : Obj.IndirectSymTable.Symbols)
     if (ISE.Symbol)
       (*ISE.Symbol)->Referenced = true;
 }
 
-static void updateAndRemoveSymbols(const CopyConfig &Config, Object &Obj) {
+static void updateAndRemoveSymbols(const CommonConfig &Config, Object &Obj) {
   for (SymbolEntry &Sym : Obj.SymTable) {
     auto I = Config.SymbolsToRename.find(Sym.Name);
     if (I != Config.SymbolsToRename.end())
       Sym.Name = std::string(I->getValue());
   }
 
   auto RemovePred = [Config, &Obj](const std::unique_ptr<SymbolEntry> &N) {
     if (N->Referenced)
static LoadCommand buildRPathLoadCommand(StringRef Path) {
   RPathLC.path = sizeof(MachO::rpath_command);
   RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size() + 1, 8);
   LC.MachOLoadCommand.rpath_command_data = RPathLC;
   LC.Payload.assign(RPathLC.cmdsize - sizeof(MachO::rpath_command), 0);
   std::copy(Path.begin(), Path.end(), LC.Payload.begin());
   return LC;
 }
 
-static Error processLoadCommands(const CopyConfig &Config, Object &Obj) {
+static Error processLoadCommands(const CommonConfig &Config, Object &Obj) {
   // Remove RPaths.
   DenseSet<StringRef> RPathsToRemove(Config.RPathsToRemove.begin(),
                                      Config.RPathsToRemove.end());
 
   LoadCommandPred RemovePred = [&RPathsToRemove,
                                 &Config](const LoadCommand &LC) {
     if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH) {
       // When removing all RPaths we don't need to care
return createStringError(errc::invalid_argument,
                              Pair.first.str().c_str());
   if (Pair.second.size() > 16)
     return createStringError(errc::invalid_argument,
                              "too long section name: '%s'",
                              Pair.second.str().c_str());
   return Error::success();
 }
 
-static Error handleArgs(const CopyConfig &Config, Object &Obj) {
-  if (Config.AllowBrokenLinks || !Config.SplitDWO.empty() ||
-      !Config.SymbolsPrefix.empty() || !Config.AllocSectionsPrefix.empty() ||
-      !Config.KeepSection.empty() || Config.NewSymbolVisibility ||
-      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
-      !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
-      !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
-      !Config.UnneededSymbolsToRemove.empty() ||
-      !Config.SetSectionAlignment.empty() || !Config.SetSectionFlags.empty() ||
-      Config.ExtractDWO || Config.LocalizeHidden || Config.PreserveDates ||
-      Config.StripAllGNU || Config.StripDWO || Config.StripNonAlloc ||
-      Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
-      Config.StripUnneeded || Config.DiscardMode == DiscardType::Locals ||
-      !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
-    return createStringError(llvm::errc::invalid_argument,
-                             "option not supported by llvm-objcopy for MachO");
-  }
-
+static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   // Dump sections before add/remove for compatibility with GNU objcopy.
   for (StringRef Flag : Config.DumpSection) {
     StringRef SectionName;
     StringRef FileName;
     std::tie(SectionName, FileName) = Flag.split('=');
     if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
       return E;
   }
static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   }
 
   if (Error E = processLoadCommands(Config, Obj))
     return E;
 
   return Error::success();
 }
 
-Error executeObjcopyOnBinary(const CopyConfig &Config,
+Error executeObjcopyOnBinary(const CommonConfig &Config, const MachOConfig &,
                              object::MachOObjectFile &In, raw_ostream &Out) {
   MachOReader Reader(In);
   Expected<std::unique_ptr<Object>> O = Reader.create();
   if (!O)
     return createFileError(Config.InputFilename, O.takeError());
 
   if (Error E = handleArgs(Config, **O))
     return createFileError(Config.InputFilename, std::move(E));
Error executeObjcopyOnBinary(const CommonConfig &Config, const MachOConfig &,
   }
 
   MachOWriter Writer(**O, In.is64Bit(), In.isLittleEndian(), PageSize, Out);
   if (auto E = Writer.finalize())
     return E;
   return Writer.write();
 }
 
-Error executeObjcopyOnMachOUniversalBinary(CopyConfig &Config,
+Error executeObjcopyOnMachOUniversalBinary(const MultiFormatConfig &Config,
                                            const MachOUniversalBinary &In,
                                            raw_ostream &Out) {
   SmallVector<OwningBinary<Binary>, 2> Binaries;
   SmallVector<Slice, 2> Slices;
   for (const auto &O : In.objects()) {
     Expected<std::unique_ptr<Archive>> ArOrErr = O.getAsArchive();
     if (ArOrErr) {
       Expected<std::vector<NewArchiveMember>> NewArchiveMembersOrErr =
           createNewArchiveMembers(Config, **ArOrErr);
       if (!NewArchiveMembersOrErr)
         return NewArchiveMembersOrErr.takeError();
       Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrErr =
           writeArchiveToBuffer(*NewArchiveMembersOrErr,
                                (*ArOrErr)->hasSymbolTable(), (*ArOrErr)->kind(),
-                               Config.DeterministicArchives,
+                               Config.getCommonConfig().DeterministicArchives,
                                (*ArOrErr)->isThin());
       if (!OutputBufferOrErr)
         return OutputBufferOrErr.takeError();
       Expected<std::unique_ptr<Binary>> BinaryOrErr =
           object::createBinary(**OutputBufferOrErr);
       if (!BinaryOrErr)
         return BinaryOrErr.takeError();
       Binaries.emplace_back(std::move(*BinaryOrErr),
                             std::move(*OutputBufferOrErr));
       Slices.emplace_back(*cast<Archive>(Binaries.back().getBinary()),
                           O.getCPUType(), O.getCPUSubType(),
                           O.getArchFlagName(), O.getAlign());
       continue;
     }
     // The methods getAsArchive, getAsObjectFile, getAsIRObject of the class
     // ObjectForArch return an Error in case of the type mismatch. We need to
     // check each in turn to see what kind of slice this is, so ignore errors
     // produced along the way.
     consumeError(ArOrErr.takeError());
 
     Expected<std::unique_ptr<MachOObjectFile>> ObjOrErr = O.getAsObjectFile();
     if (!ObjOrErr) {
       consumeError(ObjOrErr.takeError());
-      return createStringError(std::errc::invalid_argument,
-                               "slice for '%s' of the universal Mach-O binary "
-                               "'%s' is not a Mach-O object or an archive",
-                               O.getArchFlagName().c_str(),
-                               Config.InputFilename.str().c_str());
+      return createStringError(
+          std::errc::invalid_argument,
+          Config.getCommonConfig().InputFilename.str().c_str()