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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

>> 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.
>
> the problem is in how following case might be handled:
>
> --add-symbol sym1=0x100,global --add-symbol sym2=0x200,elf-specific-flag --add-symbol sym3=0x300,coff-specific-flag
>
> If we allow multiple files with different formats and allow format specific options then above legal situation 
> could not be handled: while parsing options, for elf there would be detected unknown "coff-specific" value,
> for the coff there would be detected unknown "elf-specific" value. This would result in a error for both input files. 
> i.e. it looks like weak command-line interface which does not work for legal case.
>
> From the other side, GNU objcopy does not solve this case also and the whole case looks quite specific.
> so, if we are fine with such a behavior then let it be so.

The existing ELF behaviour already recognises but does nothing with certain flags that are COFF specific. I don't think we need to change this behaviour (i.e. COFF specific flags are ignored for ELF objects and (in the future) vice versa).

>> 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?
>
> I like this suggestion which allows to remove lazy options. If we do not need to support lazy options then we could simplificate CopyConfig structures. We might use one CopyConfig for all formats:
>
>   struct CopyConfig {
>     // common options
>     
>     // ELF options
>     
>     ... 
>   };
>
> Would it be OK or you think we need separate ELFConfig, COFFConfig(like it is currently done)...?

I don't have a particular preference as to whether format-specific options are in separate configs or not. I think if we're thinking in terms of a library, it might make sense to split them up, so that a person who wants to modify COFF objects doesn't have to populate the ELF stuff, but certainly I'm open to alternatives. If we do keep separate configs, we should ensure the option split is based on whether options are truly format specific, or just not implemented, as discussed earlier (unimplemented generic options should still be in the generic config class).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277