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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:619
+
+template <class ELFT> void BinaryELFBuilder<ELFT>::initHeaderSegment() {
+  auto &ElfHdr = Obj->ElfHdrSegment;
----------------
jakehehrlich wrote:
> rupprecht wrote:
> > 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.
> You're right. I confused it with ProgramHdrSegment. I now remember that someone had the clever idea to make the ELF header an implicit segment so that the same layout algorithm that had all the bugs worked out could already be used. As for the warning I guess I knew about that issue. The `OrderedSegments.push_back(&Obj.ProgramHdrSegment)` should accomplish adding that program headers to layout when you have a PT_PHDR segment in a consistent way. The warning is a separate issue on line 1078 (in this patch) not checking if the program headers are empty before blindly setting phoff to the offset of that segment. This is in full compliance with the ELF standard because the number of program headers is still zero. It's just that no tool normally has a reason to produce an ELF that has phoff != 0 and phnum = 0 so it's normally a sign of corruption.
I tried adding that check, and several tests failed -- I might not have been checking the right thing, or maybe the tests are bad. I added a TODO on that line to investigate further.


================
Comment at: tools/llvm-objcopy/Object.cpp:592
 
+template <class ELFT> void BinaryELFBuilder<ELFT>::initFileHeader() {
+  std::fill(Obj->Ident, Obj->Ident + 16, 0);
----------------
jakehehrlich wrote:
> So I  this function is the real problem child.  We don't actually use the Ident anywhere.This is also 100% comprised of information that the ELFWriter knows but nothing else knows. We should make the ELFWriter construct the Ident and just not store it in Object.
Nice, I was able to completely remove Ident from Object.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343