[PATCH] D102277: [llvm-objcopy][NFC] Refactor CopyConfig structure - categorize options.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D102277#2779104 <https://reviews.llvm.org/D102277#2779104>, @avl wrote:

> I edited my previous comment with this part:
>
> Actually, current format specific checks verify exactly this : "avoid using of format specific options when multiple inputs in different formats are specified". That probably means that we need to keep them? (To prevent incorrect lazy options parsing)

I don't think it's a goal of llvm-objcopy to prevent users from using format-specific options when those formats are not present, or when other formats are present. GNU objcopy doesn't do this, so doing it ourselves is probably a bad idea, as it makes the tool less compatible with GNU, for no real benefit. The exception is for unimplemented options, as discussed earlier.

I dug back into the history of the lazy option parsing, and it was my original suggestion, so that we avoided the ELF-specific bits from happening for other formats. However, the only two options that are done here are --new-symbol-visibility (which is ELF specific, and doesn't need to be lazily parsed, since it's pretty trivial), and --add-symbol (which is generic, but only implemented for ELF currently). The problem with --add-symbol is not, as I previously thought, the syntax (it's identical for all file formats as far as I can tell in GNU objcopy), but rather what to do when this option is encountered. Each individual format will need its own parsing, and obviously there's no point in parsing the option for file formats that aren't used. That being said, doing that parsing at the moment is probably harmless (there are no errors in the parseNewSymbolInfo code that are format-specific).

Perhaps we should just rewrite how --add-symbol is processed. Rather than have an ELF-specific NewSymbolInfo struct, we could have a generic one which contains the symbol name, value, section and input flags in (the latter possibly converted to a generic enum value), with that bit being handled at the same time as the rest of the command-line options, and then the processing of the flags done in ELFObjcopy when the list of these is actually about to be used. That should allow us to completely drop all lazy parsing, I think.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277