[PATCH] D99055: [llvm-objcopy][NFC] remove processing of ELF specific options from common CopyConfig structure.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055