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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall, the approach seems sound. I particularly like how it removes the templates from many of our classes, and removes the unfortunate SymbolTableSectionImpl class. Have you tried extending this with new input types, to make sure this design works?

I agree with your points about the code smells, and think they should be fixed too. As to how to split it up, could the SectionWriter/SectionVisitor related changes be done first? After that a logical split would be to do the Reader-related classes, then the Writer classes, if it's possible without having to make too many intermediate changes that are just going to be thrown away.



================
Comment at: tools/llvm-objcopy/Object.cpp:77
+
+SectionVisitor::~SectionVisitor() {}
+
----------------
This should go above the previous function.


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


================
Comment at: tools/llvm-objcopy/Object.cpp:966
     Offset += sizeof(Elf_Shdr);
-    if (this->WriteSectionHeaders)
-      Section->NameIndex = this->SectionNames->findIndex(Section->Name);
-    Section->finalize();
+    if (WriteSectionHeaders) {
+      Section.NameIndex = Obj.SectionNames->findIndex(Section.Name);
----------------
Braces here aren't needed.


================
Comment at: tools/llvm-objcopy/Object.cpp:1060
+  BufPtr = std::move(*BufferOrErr);
+  SecWriter = llvm::make_unique<SectionWriter<ELF64LE>>(*BufPtr);
 }
----------------
This line looks a little dodgy. Doesn't that mean we'll write ELF64LE sections to binary output, no matter what the input was?


================
Comment at: tools/llvm-objcopy/Object.h:76-82
+class Writer {
+public:
+  virtual ~Writer();
+
+  virtual void finalize() = 0;
+  virtual void write() = 0;
+};
----------------
It would probably make more sense for this to come after SectionVisitor.


================
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;
 
----------------
What items are in this namespace that means we need this using in a header?


================
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;
----------------
This will require us to always have the ELFReader allocated until we write, which seems a little less than ideal?


================
Comment at: tools/llvm-objcopy/Object.h:518
 
-  uint64_t TotalSize;
+protected:
+  std::vector<SecPtr> Sections;
----------------
Do you plan on having any child classes? Otherwise, why protected?


================
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();
----------------
Any particular reason this is no longer in its own function?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:317
   if (!AddGnuDebugLink.empty()) {
-    Obj->addGnuDebugLink(AddGnuDebugLink);
+    auto Sec = Obj.addSection<GnuDebugLinkSection>(StringRef(AddGnuDebugLink));
   }
----------------
Unused return value and unnecessary braces.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:329
 
+// TODO: uh...this is complicated.
 int main(int argc, char **argv) {
----------------
What's complicated? :)


Repository:
  rL LLVM

https://reviews.llvm.org/D42222