[PATCH] D42222: [llvm-objcopy] Refactor llvm-objcopy to use reader and writer objects

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:667
+// A generic size function which computes sizes of any random access range.
+template <class R> size_t size(R &&Range) {
+  return static_cast<size_t>(std::end(Range) - std::begin(Range));
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > I'm not sure you want this to take an r-value reference? Shouldn't it be a const reference, since you don't need to change the range?
> It's not an rvalue reference, it's a "universal reference". It's whatever the user passes it.
Huh, I can't say I was aware of the distinction - I had to go and look it up!


================
Comment at: tools/llvm-objcopy/Object.cpp:1060
+  BufPtr = std::move(*BufferOrErr);
+  SecWriter = llvm::make_unique<SectionWriter<ELF64LE>>(*BufPtr);
 }
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > This line looks a little dodgy. Doesn't that mean we'll write ELF64LE sections to binary output, no matter what the input was?
> Ah yes, I remember having this dilemma before pushing it to the side in favor of progress. I was thinking about splitting up SectionWriter into a non-ELFT-templated base class and a separate ELFT-template base class and then *also* a binary section writer (that is non-ELFT-templated) that throws errors for the methods that need ELFT to output correctly. How does that sound?
I think that would make things a little clearer, yes. I'd like to see which methods you're referring to that should throw errors, and why we don't need them for a binary section writer though.


================
Comment at: tools/llvm-objcopy/Object.h:499
 
-template <class ELFT> class ELFObject : public Object<ELFT> {
-private:
-  using SecPtr = std::unique_ptr<SectionBase>;
-  using SegPtr = std::unique_ptr<Segment>;
-
-  using Elf_Shdr = typename ELFT::Shdr;
-  using Elf_Ehdr = typename ELFT::Ehdr;
-  using Elf_Phdr = typename ELFT::Phdr;
+using namespace object;
 
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > What items are in this namespace that means we need this using in a header?
> ELFFile, ELFObjectFile, and maybe something else I'm not quite remembering. I should probably just explicitly use "using" on those things I need rather than massively pollute the whole namespace, huh?
Yes, I think that's probably right in a header file (no problem in cpp files, mind you).


================
Comment at: tools/llvm-objcopy/Object.h:503-505
+  // We keep this around so that Objects can be constructed from objects owned
+  // by the binary object and they won't be deallocated.
+  OwningBinary<Binary> Binary;
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > This will require us to always have the ELFReader allocated until we write, which seems a little less than ideal?
> I want to keep it so that the data between the Object and the Binary is not duplicated. I do agree that it doesn't seem like the Reader shouldn't *have* to stay open the whole time but I'd like the Binary to stay open the whole time (until all output files have been written at least). I think this can be remedied in two opposite ways. One way is for the Reader to maintain ownership of the Objects so that the Binary and the Objects that reference it's data have a shared lifetime. The other idea is that this could perhaps be decoupled and the Reader could store a reference to the Binary. It then becomes the user's job to make sure that the Bianry stays open as long as any object created by the Reader. Right now it's (oddly) the user's job to make sure that the Reader stays open as like as any object created by it. I agree that's wrong.
> 
> What are your thoughts?
I'm actually wondering whether we could transfer ownership of either the Reader, or just the Binary associated with it to the Object. That way the original Reader can be disposed of at any point after creating the Object. Unfortunately, this doesn't play particularly well with multiple input types. Of the two ideas you suggested I'd prefer that Readers have ownership of their Objects.


Repository:
  rL LLVM

https://reviews.llvm.org/D42222