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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In https://reviews.llvm.org/D50343#1190050, @jakehehrlich wrote:

> 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.


Yeah, it's not my first choice either, but it did make the implementation easier and seemed somewhat consistent with the pattern elsewhere in this file.

I wasn't sure what plans you had for refactoring things here (still haven't seen whatever thread was referenced earlier). My hunch is that "the right way" would be to not have any extra ELF type here and just always directly use what's in  Object/ELFTypes.h, and to do things with composition instead of templated-inheritance, and I'm happy to make that change (or whatever alternative change makes sense), but that seems like an orthogonal discussion that isn't related to the features implemented here.



================
Comment at: tools/llvm-objcopy/Object.cpp:619
+
+template <class ELFT> void BinaryELFBuilder<ELFT>::initHeaderSegment() {
+  auto &ElfHdr = Obj->ElfHdrSegment;
----------------
jakehehrlich wrote:
> 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.
I just verified this; commenting out this section causes an invalid object file (readelf/llvm-readobj can't read it at all).
This is ElfHdrSegment. I think you are confusing this with ProgramHdrSegment?

However, speaking of program headers, line 1290 (i.e. `OrderedSegments.push_back(&Obj.ProgramHdrSegment)`) seems to do nothing -- all the tests pass -- and also gets rid of the error `readelf: Warning: possibly corrupt ELF header - it has a non-zero program header offset, but no program headers` when running readelf -a on object files that llvm-objcopy produces. I'd like to take a look at that after this patch; it seems to be a preexisting issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343