[PATCH] D56211: [llvm-objcopy][ELF] Implement a mutable section visitor that updates size-related fields (Size, EntrySize, Align) before layout.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with a couple of minor comments.

I am somewhat concerned that we're adding another loop over all the sections, as this could be somewhat slow for very large objects, but it's probably a silly concern. However, it might be nice to move the section sizing later, to the point where all sections have been added, into the Index setting loop. Otherwise, when we are adding any additional sections we have to ensure that their sizes are correct, whereas the section sizer could do that all for us. I don't mind really though.



================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:89
+  Sec.Size = Sec.Symbols.size() * Sec.EntrySize;
+  Sec.Align = sizeof(Elf_Shdr::sh_addralign);
+}
----------------
I assume here and below, sh_addralign is used as a placeholder for "whatever is the largest data field in the struct" (i.e. sh_size or whatever would be equally valid)?


================
Comment at: tools/llvm-objcopy/ELF/Object.h:91
+public:
+  virtual ~MutableSectionVisitor() = default;
+
----------------
I'm a little troubled with how similar this is to SectionVisitor, but I don't have a better solution overall. However, I would like it for the destructors to be defined in the same manner. At the moment this one is defaulted, and the other is defined out-of-line.


================
Comment at: tools/llvm-objcopy/ELF/Object.h:156-157
+public:
+  ELFSectionSizer() = default;
+  virtual ~ELFSectionSizer() = default;
+  void visit(Section &Sec) override;
----------------
I'm not sure you need either of these? The class has no members, so these should be trivially generated by the compiler with no need to be explicit. I think (I might be misremembering my C++...)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56211/new/

https://reviews.llvm.org/D56211