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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> 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.

> 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)...?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277