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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:575
 
-static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
+static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Reader &Reader,
                                       Buffer &Out) {
----------------
jakehehrlich wrote:
> rupprecht wrote:
> > jakehehrlich wrote:
> > > This change (which is not necessarily wrong) conflicts with one of the two proposals I made for how --dump-section should work in another change. After a bit of thought I think "ExecuteElfObjcopyOnBlob" might be needed or something. The --dump-section change hasn't really shaken out just yet so we'll see.
> > I renamed this to ExecuteElfObjcopyOnBlob since it's no longer always a binary -- is that what you meant?
> > Do you have a link to your proposals on --dump-section?
> No I meant for ExecuteElfObjcopyOnBinary to remain unchanged and to not take a Reader at all but instead for a separate almost identical function  ExecuteElfObjcopyOnBlob to exist so that current invocations to ExecuteElfObjcopyOnBinary would have to decide which one to call. The issue is that sometimes there are options (like --dump-section) which only really make sense on the original data. Depending on how that all shakes out we might want to handle that option differently depending on if -I binary or -I elf* was used. One of the options (and the option I'm currently in favor of) for that change would allow us to treat this all uniformly however.
Sorry, I'm still not following. Do you mean this method should be duplicated, once for ELF binary, once for non-ELF binaries?

```
// Original method, always handles ELF binary input
static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, FileBuffer &Binary,
                                      Buffer &Out) {
  ELFReader Reader(&Binary);
  std::unique_ptr<Object> Obj = Reader.create();

  HandleArgs(Config, *Obj, Reader, Reader.getElfType());
  // ... maybe some special logic only for ELF binaries ...

  std::unique_ptr<Writer> Writer =
      CreateWriter(Config, *Obj, Out, Reader.getElfType());
  Writer->finalize();
  Writer->write();
}

// Called for -I Binary
static void ExecuteElfObjcopyOnBlob(const CopyConfig &Config, MemoryBuffer &Blob,
                                    Buffer &Out) {
  BinaryReader Reader(&Blob);
  std::unique_ptr<Object> Obj = Reader.create();

  ElfType OutputElfType = <... get ELFType from -B flag ...>
  HandleArgs(Config, *Obj, Reader, ET, OutputElfType);
  // ... maybe some special logic only for non-ELF binaries ...

  std::unique_ptr<Writer> Writer =
      CreateWriter(Config, *Obj, Out, OutputElfType);
  Writer->finalize();
  Writer->write();
}
```


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:291
 
+static Optional<ElfType> GetOutputElfType(const CopyConfig &Config,
+                                          const Reader &Reader) {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50117