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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D99055#2700994 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055