[PATCH] D54939: [RFC] [llvm-objcopy] Initial COFF support

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm not really a COFF expert, so I haven't yet looked at the details of this change. It broadly looks like the right direction though.

If I get time, I'll try to start reviewing the details, but it would be better if somebody with more familiarity looked at them.



================
Comment at: include/llvm/Object/COFF.h:974
   }
+  std::error_code getCOFFHeader(const coff_file_header *&Res) const {
+    Res = COFFHeader;
----------------
We seem to be mixing our styles of defining things in headers and in .cpp files for no particular reason. Would you mind defining these in COFF.cpp, please since there are two that already are? (I'd support moving getDosHeader into the COFF.cpp too in that case).

I could be persuaded instead that the two PE header functions should be moved here.


================
Comment at: test/tools/llvm-objcopy/COFF/basic-copy.test:25
+  (lld currently can leave a whole empty sector inbetween)
+- The actual layout of of the string table (it can be filled linearly,
+  strings can be dedupliated, the table can be optimized by sharing tails
----------------
nit: remove the extra "of"


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:47
+
+std::unique_ptr<Object> COFFReader::create() const {
+  auto Obj = llvm::make_unique<Object>();
----------------
This is quite a big function. Would it be feasible to split it up into a few helpers?


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:144
+template <class CoffHeaderTy, class PeHeaderTy, class SymbolTy>
+void COFFWriter::finalize(CoffHeaderTy &CoffFileHeader, PeHeaderTy &PeHeader) {
+  size_t SizeOfHeaders = 0;
----------------
Again, this is quite a big function. Would it be feasible to split it up into a few helpers?


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:240
+template <class CoffHeaderTy, class PeHeaderTy, class SymbolTy>
+void COFFWriter::write(CoffHeaderTy &CoffFileHeader, PeHeaderTy &PeHeader) {
+  finalize<CoffHeaderTy, PeHeaderTy, SymbolTy>(CoffFileHeader, PeHeader);
----------------
Again, this is quite a big function. Would it be feasible to split it up into a few helpers?


================
Comment at: tools/llvm-objcopy/COFF/Object.h:30
+
+class Object;
+
----------------
To avoid confusion, maybe we should rename the ELF Object to ELFObject, and then this Object to COFFObject, etc?


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

https://reviews.llvm.org/D54939