[PATCH] D116556: [llvm-objcopy] Implement the PE-COFF specific --subsystem option

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:598
       !Common.SymbolsToAdd.empty()) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
----------------
avl wrote:
> mstorsjo wrote:
> > MaskRay wrote:
> > > mstorsjo wrote:
> > > > On the topic of testing diagnostics... Here (in the `get*Config()` format specific methods) we do error out if a common option has been set, that isn't implemented for that format. But what if options in one of the different format specific structs have been set, e.g. an option set in `ELF`, but we end up calling `getMachOConfig()` - then we don't error out at all.
> > > > 
> > > > Should we add a separate `bool ELFOptionsSet` (and similar for all other formats) which we could check for in the `get*Config()` methods, so we error out cleanly in the same way, if format specific options are set for the wrong format?
> > > Personally I think no action item is needed for you on this one:)
> > > 
> > > The diagnostic is untested... I remember that when we add recent new binary-format-specific options, we don't bother add tests for other binary formats. I would not bother adding a diagnostic for non-COFF if I am to implement the feature:)
> > > 
> > > This part may use some refactoring which can make such tests less necessary, but that will be out of the scope of this patch.
> > Yeah it's out of scope for this patch as we don't have it so far, but more as a general question whether we should have it.
> > 
> > Sure, these diagnostics are untested, but I still think they're kinda important (so maybe we should add at least some, but not exhaustive, tests for it?)
> > 
> > But maybe the common options are more important to bail out on, rather than format specific options which one doesn't end up using for incorrect formats quite as easily, contrary to wanting to use unimplemented features.
> It was a design decision that warning message should be displayed for common but not yet implemented option and warning message should NOT be displayed for format specific option set with other formats: https://reviews.llvm.org/D102277#2772972 . That way, using of format specific options when multiple inputs in different formats are specified - would be handled without error messages. 
Ok, fair enough.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOpts.td:110
+    : Eq<"subsystem",
+         "[COFF only] Set PE subsystem and version">,
+      MetaVarName<"name[:version]">;
----------------
avl wrote:
> we do not have similar description for other format specific options : "[COFF only]". It looks useful, but probably it would be better to make it as a separate patch for all format specific options. 
Ok, I'll remove it from here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556