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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

> Have you tried extending this with new input types, to make sure this design works?

No but I was thinking about the case while designing this approach. I should probably actually do that just to make sure. In principal it should just be as simple as making a new Reader class and constructs all the needed parts.



================
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));
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:1060
+  BufPtr = std::move(*BufferOrErr);
+  SecWriter = llvm::make_unique<SectionWriter<ELF64LE>>(*BufPtr);
 }
----------------
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?


================
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;
 
----------------
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?


================
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;
----------------
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?


================
Comment at: tools/llvm-objcopy/Object.h:518
 
-  uint64_t TotalSize;
+protected:
+  std::vector<SecPtr> Sections;
----------------
jhenderson wrote:
> Do you plan on having any child classes? Otherwise, why protected?
Good, point. I don't think I'll ever have any child classes. It was mostly holdover from the previous Object type.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:175-182
+  if (!SplitDWO.empty()) {
+    auto DWOFile = Reader.create();
+    DWOFile->removeSections(
+        [&](const SectionBase &Sec) { return OnlyKeepDWOPred(*DWOFile, Sec); });
+    auto Writer = CreateWriter(*DWOFile);
+    Writer->finalize();
+    Writer->write();
----------------
jhenderson wrote:
> Any particular reason this is no longer in its own function?
Not that I remember, I'll put it back.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:329
 
+// TODO: uh...this is complicated.
 int main(int argc, char **argv) {
----------------
jhenderson wrote:
> What's complicated? :)
Haha, that was a comment to my self that I left in, whoops. I belive that was one of the "update to use reader and writer" todos that I wasn't 100% clear on how to acomplish.


Repository:
  rL LLVM

https://reviews.llvm.org/D42222