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

Tue May 25 01:37:31 PDT 2021

jhenderson added a comment.

Taking the conversation out of line.

>> My gut says that our "unsupported option" handling needs redoing. Some of the options are applicable for multiple formats, but not yet implemented. Such options belong in the CommonConfig, which means we can cleanly reference them from within the file-format specific configs. I don't think we should be warning for other options. E.g. if a user uses an option that is tied to ELF, such as the new symbol visibility option, because the concept is not applicable in other formats, it shouldn't be checked for. This then means we can avoid referencing file format specific configs from each other, which I think is a good design.
>>
>> What do you think?
>
> My understanding is: whether such change is OK or not depends on the usage idea of the llvm-objcopy tool(and friends llvm-strip...). Is it supposed to work with multiple formats at a time?

Short answer: yes the tools are supposed to work with multiple formats in a single run. llvm-objcopy/llvm-strip need to be compatible with GNU objcopy/strip. I just ran a quick experiment and GNU objcopy is able to handle inputs with different file formats in the same run (I specifically tested COFF and ELF). As such, llvm-objcopy needs to be able to handle the same cases.

For reference, I have heard of instances in the past where people create a single archive containing files of different formats, so that they can use it in a cross-platform manner. The specific example I know of was for an ELF32 target and an ELF64 target, but I believe the principle is equally applicable for COFF, Mach-O etc.

I did a quick experiment with GNU objcopy and couldn't immediately identify any options that were specific to ELF (I didn't look too hard though), but there are some PE-specific options, and these don't generate errors when run on an ELF object. As such, my earlier comment about warning/error-ing for unimplemented options that apply, and being silent for other options would be the most GNU compatible.

================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:556-557
+  // Parse lazy options.
+  if (!AreELFLazyOptionsParsed) {
+    AreELFLazyOptionsParsed = true;

----------------
avl wrote:
> jhenderson wrote:
> > I really don't like this boolean here. We have Optional for a reason, and it does exactly this without the need of the additional boolean, right?
> > 
> > My gut says that our "unsupported option" handling needs redoing. Some of the options are applicable for multiple formats, but not yet implemented. Such options belong in the CommonConfig, which means we can cleanly reference them from within the file-format specific configs. I don't think we should be warning for other options. E.g. if a user uses an option that is tied to ELF, such as the new symbol visibility option, because the concept is not applicable in other formats, it shouldn't be checked for. This then means we can avoid referencing file format specific configs from each other, which I think is a good design.
> > 
> > What do you think?
> > I really don't like this boolean here. We have Optional for a reason, and it does exactly this without the need of the additional boolean, right?
> > 
> Let`s temporarily delay this point.
> 
> > My gut says that our "unsupported option" handling needs redoing. Some of the options are applicable for multiple formats, but not yet implemented. Such options belong in the CommonConfig, which means we can cleanly reference them from within the file-format specific configs. I don't think we should be warning for other options. E.g. if a user uses an option that is tied to ELF, such as the new symbol visibility option, because the concept is not applicable in other formats, it shouldn't be checked for. This then means we can avoid referencing file format specific configs from each other, which I think is a good design.
> > 
> > What do you think?
> 
> My understanding is: whether such change is OK or not depends on the usage idea of the llvm-objcopy tool(and friends llvm-strip...). Is it supposed to work with multiple formats at a time? 
> 
> I think, the intention of such checks : 
> 
> 
> ```
>   if (MachO.StripSwiftSymbols || MachO.KeepUndefined)
>     return createStringError(llvm::errc::invalid_argument,
>                              "option not supported by llvm-objcopy for ELF");
> ```
> 
> is to avoid multi-format usage: 
> 
> 
> ```
> llvm-strip -S -T ELF-file MachO-file
> llvm-strip: error: option not supported by llvm-objcopy for ELF
> ```
> 
> If we will avoid format-specific checks then we would allow multi-format usages. 
> 
> Is it OK?
> 
> If we would like to preserve the idea of avoiding multi-format usages and we are fine with redoing "unsupported options" handling then it might be better to implement that idea explicitly(if I did not miss something):
> 
> 1. Before starting to parse options, detect the format of the input file(single source file, first source file, first file inside an archive).
> 2. Parse options for the detected format and display warning if there were specified options for another format (other than we detected at step 1.).
> 3. while handling source file(s), ignore(or fail?) files which have a format different from what we detected at stage 1.
> 4. while handling source file(s), check options from CommonConfig: If there are some unimplemented yet - then display the warning message.
> 
> Such an approach, explicitly force only single format usage(which looks like a good thing). Also, it avoids the necessity to implement lazy options, which complicate interfaces and implementation.
> 
> What do you think?
> > I really don't like this boolean here. We have Optional for a reason, and it does exactly this without the need of the additional boolean, right?
> > 
> Let`s temporarily delay this point.
> 
> > My gut says that our "unsupported option" handling needs redoing. Some of the options are applicable for multiple formats, but not yet implemented. Such options belong in the CommonConfig, which means we can cleanly reference them from within the file-format specific configs. I don't think we should be warning for other options. E.g. if a user uses an option that is tied to ELF, such as the new symbol visibility option, because the concept is not applicable in other formats, it shouldn't be checked for. This then means we can avoid referencing file format specific configs from each other, which I think is a good design.
> > 
> > What do you think?
> 
> My understanding is: whether such change is OK or not depends on the usage idea of the llvm-objcopy tool(and friends llvm-strip...). Is it supposed to work with multiple formats at a time? 
> 
> I think, the intention of such checks : 
> 
> 
> ```
>   if (MachO.StripSwiftSymbols || MachO.KeepUndefined)
>     return createStringError(llvm::errc::invalid_argument,
>                              "option not supported by llvm-objcopy for ELF");
> ```
> 
> is to avoid multi-format usage: 
> 
> 
> ```
> llvm-strip -S -T ELF-file MachO-file
> llvm-strip: error: option not supported by llvm-objcopy for ELF
> ```
> 
> If we will avoid format-specific checks then we would allow multi-format usages. 
> 
> Is it OK?
> 
> If we would like to preserve the idea of avoiding multi-format usages and we are fine with redoing "unsupported options" handling then it might be better to implement that idea explicitly(if I did not miss something):
> 
> 1. Before starting to parse options, detect the format of the input file(single source file, first source file, first file inside an archive).
> 2. Parse options for the detected format and display warning if there were specified options for another format (other than we detected at step 1.).
> 3. while handling source file(s), ignore(or fail?) files which have a format different from what we detected at stage 1.
> 4. while handling source file(s), check options from CommonConfig: If there are some unimplemented yet - then display the warning message.
> 
> Such an approach, explicitly force only single format usage(which looks like a good thing). Also, it avoids the necessity to implement lazy options, which complicate interfaces and implementation.
> 
> What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277