[PATCH] D99055: [llvm-objcopy] Refactor CopyConfig structure.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D99055#2660459 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D67139> suggested dividing options into format-specific parts. Thus this patch tries to resolve points 1 and 2 and to follow D67139 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D67139>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055