[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 inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:206
 static std::unique_ptr<Writer> CreateWriter(const CopyConfig &Config,
-                                            Object &Obj, Buffer &Buf,
-                                            ElfType OutputElfType) {
+                                            const Reader &Reader, Object &Obj,
+                                            Buffer &Buf) {
----------------
jakehehrlich wrote:
> rupprecht wrote:
> > jhenderson wrote:
> > > I feel like @jakehehrlich, @alexshap and I had a discussion about this a number of months ago, but I don't really remember what we agreed etc. My current instinct is that a Writer (and the functions used to create it) shouldn't need to know about anything to do with the Reader. This change couples the two together, which I don't really like. I'd rather it be decided outside the CreateWriter function and passed in (as was the case before).
> > > 
> > > If the problem is the assumption of needing an "ElfType", maybe we should generalise that enum to not be specific to do with ELFs?
> > > I feel like @jakehehrlich, @alexshap and I had a discussion about this a number of months ago, but I don't really remember what we agreed etc. 
> > I've read through the comments at D41687 (Jake pointed me to that one as a starter for this change), but let me know if there's some other discussion thread you had that I should read first.
> > 
> > > My current instinct is that a Writer (and the functions used to create it) shouldn't need to know about anything to do with the Reader. This change couples the two together, which I don't really like. I'd rather it be decided outside the CreateWriter function and passed in (as was the case before).
> > That's true for CreateWriter itself, but currently ExecuteElfObjcopyOnBinary (which is constructs a writer via CreateWriter) has to look at the ELFReader it has created so it knows what elf type to pass.
> > 
> > Also -- this problem could be pushed back up another layer, i.e. Writer could be passed into ExecuteElfObjcopyOnBinary instead of ExecuteElfObjcopyOnBinary calling CreateWriter itself... although there is currently another chain of ExecuteElfObjcopyOnBinary -> HandleArgs -> SplitDWOToFile -> CreateWriter that would also need to be plumbed.
> > 
> > > 
> > > If the problem is the assumption of needing an "ElfType", maybe we should generalise that enum to not be specific to do with ELFs?
> > > 
> > 
> > Reading from a binary input should still have an elf type, it will just come from the -B command line flag, instead of being inferred from the object file magic header.
> > 
> > However -- refactoring to your suggestion so that CreateWriter doesn't need to know about Reader, I've chosen to change it to Optional<ElfType>. If the input and output are both "binary" (I'm not sure if that's a valid use of objcopy, but it's at least allowed by command line flags), then there is no elf type.
> My feeling is that the reader and writer should be wholly independent but that you might use information from the reader (or other sources) in order to select which writer you use by default. For instance it should be the case that the same output writer type is chosen for the same input reader by default. If `-I binary` is used  however that goes out the window and we force the user to specify. Indeed if the user specifies anything we should listen to the user.
> 
> For instance I thought of a use case for converting from ELF64 to ELF32 (which is something I previously said would never happen). The multi boot specification specifies how to load ELF32 but not ELF64 but all existing tools only support producing ELF64 binaries for 64-bit architectures. Generally this is solved by creating a small 32-bit binary that contains the 64-bit binary as a blob (a la `-O binary` in most cases) but I *think* there's no real reason you can't just convert a 64-bit binary (containing only data and program headers and some meaningless sections like .text) and directly place it in a 32-bit binary. This would work by having the user specify `-O elf32-x86_64` or something like that.
> 
> I like passing `Optional<ElfType>` as a means of accomplishing this.
> I've read through the comments at D41687 (Jake pointed me to that one as a starter for this change), but let me know if there's some other discussion thread you had that I should read first.

It was a fairly length email chain we had between ourselves off-list. Assuming @jakehehrlich and @alexshap are happy, I see no reason not to forward it to you.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:291
 
+static Optional<ElfType> GetOutputElfType(const CopyConfig &Config,
+                                          const Reader &Reader) {
----------------
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 ?
If it's unrecoverable, should we actually return an Expected?


Repository:
  rL LLVM

https://reviews.llvm.org/D50117