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

[Jordan Rupprecht via Phabricator via llvm-commits]

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:
> > > rupprecht wrote:
> > > > 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.
> > > How did the tests fail? Some of the tests do a literal check against the header. e.g. they may be checking to see that 0x40 is used even when (to comply with the warning) 0x0 should be used instead.
> > With this change:
> > ```
> >   // Obj.ProgramHdrSegment.firstSection() == nullptr implies
> >   // Obj.ProgramHdrSegment.Sections is empty
> >   Ehdr.e_phoff = Obj.ProgramHdrSegment.firstSection() == nullptr
> >                      ? 0
> >                      : Obj.ProgramHdrSegment.Offset;
> > ```
> > 
> > The failures usually looked like:
> > 
> > ```
> > Command Output (stderr):
> > --
> > <src>/llvm/test/tools/llvm-objcopy/triple-overlap.test:72:14: error: CHECK-NEXT: expected string not found in input
> > #CHECK-NEXT: Type: PT_LOAD (0x1)
> >              ^
> > <stdin>:9:2: note: scanning from here
> >  Type: (0x464C457F)
> >  ^
> > <stdin>:21:2: note: possible intended match here
> >  Type: (0x400004)
> >  ^
> > ```
> > 
> > Actually, it looks like the object is corrupted with that change, e.g. the first program header for that test has an alignment of 15762873573703680, vs 4096 for all of them on the base side.
> Yeah that's not the right check. Use `Obj.ProgramHeaders.size() == 0`
I can't seem to get that to work:
- Obj doesn't have a field called ProgramHeaders
- You probably mean `Obj.ProgramHdrSegment`, but `Segment` doesn't have a method size()
- If I implement size() as returning either `Contents.size()` or `Sections.size()` and then try:
```
  Ehdr.e_phoff =
      Obj.ProgramHdrSegment.size() == 0 ? 0 : Obj.ProgramHdrSegment.Offset;
```
Then I get the same failure as before


Repository:
  rL LLVM

https://reviews.llvm.org/D50343