[PATCH] D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

@alexshap, are you happy for me to forward the email thread we had about refactoring llvm-objcopy to @rupprecht?



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:291
 
+static Optional<ElfType> GetOutputElfType(const CopyConfig &Config,
+                                          const Reader &Reader) {
----------------
alexshap wrote:
> jhenderson wrote:
> > rupprecht wrote:
> > > alexshap wrote:
> > > > rupprecht wrote:
> > > > > alexshap wrote:
> > > > > > it looks like now or even in the future None  here is not "recoverable" anyway
> > > > > > (down the line we exit with an error),
> > > > > > maybe it would be better to return the enum directly instead ?
> > > > > > (and if the output format is "binary" it will error("Unsupported output format ...")).
> > > > > The unrecoverable error that's thrown when this optional doesn't have a value comes immediately after a check that Config.OutputFormat == "binary". So returning None is valid in that case.
> > > > yes, that's exactly what i was saying -  but the point was different - wouldn't returning enum directly make the code a bit simpler ?
> > > I think it would be OK to return an error/return Expected<ElfType>/etc. if this were called from a scope where we were guaranteed that the output is an elf binary. But, it's not. I think if the output format is "binary", the logical thing for an API to do when asked "What's your elf type?" in a generic context would be return <None>, because the output isn't an ELF binary at all, right?
> > > 
> > > i.e., the relevant code distilled is:
> > > ```
> > > static void ExecuteElfObjcopyOnBinary() {
> > >   ...
> > >   Optional<ElfType> OutputElfType = GetOutputElfType();
> > >   std::unique_ptr<Writer> Writer = CreateWriter(..., Config, OutputElfType);
> > >   ...
> > > }
> > > 
> > > static std::unique_ptr<Writer>CreateWriter(..., Config, Optional<OutputElfType>) {
> > >   if (Config.OutputFormat == "binary") // Return binary writer, and OutputElfType is never even looked at
> > >   else // ... Use OutputElfType to make an ELFWriter ...
> > > }
> > > ```
> > > 
> > > Having GetOutputElfType() return an error if the output is binary will break that case. Having it return Expected<ElfType> would work, although it would be weird -- you'd have to only check it in CreateWriter only after you've checked that the output format isn't binary, so I think Optional<ElfType> is cleaner.
> > > 
> > > Given the state the code is in today, it's always possible to derive an ELF type, but only because --input-target is currently ignored, and the inputs are implicitly assumed to be ELF objects from which we can derive a type. I think the code is simpler when optimized for reading, and returning an Optional<ElfType> makes it clear that the output might not be ELF.
> > > 
> > > Sorry for the long reply -- I'm OK with making some changes here, but I'm not sure which direction you want this to go in.
> > If it's unrecoverable, should we actually return an Expected?
> though this is probably not a big deal, i think Optional is not the right choice here because of the reasons mentioned above. I think it would be better to either return enum directly or make use of Expected<...>.
If I see a function called `GetOutputElfType` I expect it to return a type of ELF. If the function returns anything else (apart from an error indicator of some kind), then it's no longer an "ElfType", so the function is badly named. If we only expect this to be used when we want an ELF, then the return should be Expected, because anything else would be unexpected, and therefore an error. On the other hand, if this function is intended to be more general, then it should be renamed to simply `GetOutputType`. I'd still say returning None is wrong, because that would imply that we don't want an output at all. Rather, I'd say an explicit enum is the right response (i.e. return something like ELF64LE/ELF32BE/Binary etc).

Related note: None is not an indicator of error, it is an indicator of No Result being a reasonable response. If this function remains as GetOutputElfType and it is impossible to determine what kind of ELF it was, then a None result might be acceptable.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:631-632
     MemBuffer MB(ChildNameOrErr.get());
-    ExecuteElfObjcopyOnBinary(Config, **ChildOrErr, MB);
+    ELFReader Reader(ChildOrErr->get());
+    ExecuteElfObjcopyOnBinary(Config, Reader, MB);
 
----------------
I may have missed this somewhere in the discussion, but why are we moving the Reader creation out of the ExecuteElfObjcopyOnBinary function?


Repository:
  rL LLVM

https://reviews.llvm.org/D50117