[PATCH] D50343: [llvm-objcopy] Add support for -I binary -B <arch>.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.



================
Comment at: tools/llvm-objcopy/Object.cpp:619
+
+template <class ELFT> void BinaryELFBuilder<ELFT>::initHeaderSegment() {
+  auto &ElfHdr = Obj->ElfHdrSegment;
----------------
Correct me if I'm wrong but I don't think GNU objcopy produces any program headers when it does this. The resulting file should be relocatable and thus program headers are pointless. Even if GNU objcopy does this, we probably shouldn't.


================
Comment at: tools/llvm-objcopy/Object.cpp:652
+      MemBuf->getBufferSize());
+  auto &DS = Obj->addSection<OwnedDataSection>(".data", AR);
+
----------------
The MemBuf should outlive the section in which case you can just use `Section`


================
Comment at: tools/llvm-objcopy/Object.cpp:656
+
+  std::string S = "_binary_" + MemBuf->getBufferIdentifier().str();
+  std::replace_if(std::begin(S), std::end(S),
----------------
nit: Can you use a Twine? You'll have to use an std::string for the buffer identifier part unfortunately but other than that you should be able to use a twine.


================
Comment at: tools/llvm-objcopy/Object.cpp:676
+  for (auto &Section : Obj->sections()) {
+    if (&Section == Obj->SymbolTable)
+      continue;
----------------
I think you're ok to initialize the symbol table in whatever order you want here since there are no relocation. That's a current thorn that you even have to initialize things in a different order. I need to carve out some time to refactor llvm-objcopy to work differently to get there however.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343