Page MenuHomePhabricator

[llvm-objcopy] Fix bug in how segment alignment was being handled
ClosedPublic

Authored by jakehehrlich on Oct 20 2017, 11:43 AM.

Details

Summary

Just aligning segment offsets to segment alignment is incorrect and also wastes more space than is needed. The requirement is that p_offset == p_addr modulo p_align *not* that p_offset == 0 modulo p_align. Generally speaking we've been using p_addr == 0 modulo p_align. In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers. This change fixes this bad behavior in llvm-objcopy.

Diff Detail

Repository
rL LLVM

Event Timeline

jakehehrlich created this revision.Oct 20 2017, 11:43 AM
jhenderson edited edge metadata.Oct 23 2017, 2:11 AM

You should probably have a test for the BinaryObject case as well.

In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers

Could you explain a bit more what you mean by this? The p_align field for a segment should be completely independent of the align field of its sections.

test/tools/llvm-objcopy/check-addr-offset-align.test
15 ↗(On Diff #119674)

This might be related to the problem below, but this Address (and that of .data) does not lie within the corresponding segment, according to the VAddr field you specify for the segment, so I'm not sure this makes any sense.

43–44 ↗(On Diff #119674)

Isn't this the wrong behaviour here?

p_offset = 0x230, p_vaddr = 0x1030, p_align = 0x1000, so (p_offset % p_align) != (p_vaddr % p_align).

I'd expect offset to be 0x1030 in this case.

You should probably have a test for the BinaryObject case as well.

In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers

Could you explain a bit more what you mean by this? The p_align field for a segment should be completely independent of the align field of its sections.

I think this should be the case but I settled for allowing it to be computed from the sections at the request of the reviewer. I have a change in yaml2obj to make this the case up for review. Hopefully it gets reviewed soon.

Fixed the test case with all the strange addresses that made no sense.

jhenderson added inline comments.Oct 31 2017, 4:11 AM
test/tools/llvm-objcopy/check-addr-offset-align.test
2–3 ↗(On Diff #120873)

Umm... reading this again, and I realise now that the test is actually testing yaml2obj, not llvm-objcopy!

You are using binary output to %t2, but then using readobj to check %t, which is the output of yaml2obj!

Also, there should really be test cases for both ELF and binary output format cases, since this change affects both.

  1. Added test for -O binary case
  2. That test still made no sense. It should test the fact that even though the second segment has alignment 0x1000 it can't still have offset 0x1008.
jakehehrlich added inline comments.Oct 31 2017, 5:57 PM
test/tools/llvm-objcopy/check-addr-offset-align.test
2–3 ↗(On Diff #120873)

Yikes! It should be fixed now.

jhenderson accepted this revision.Nov 1 2017, 3:38 AM

LGTM with the comment nits fixed.

tools/llvm-objcopy/Object.cpp
673 ↗(On Diff #121096)

There's an extra space here between "that" and "(Offset + ...".

678 ↗(On Diff #121096)

"...Offset however so..." -> "...Offset, however, so..."

This revision is now accepted and ready to land.Nov 1 2017, 3:38 AM
This revision was automatically updated to reflect the committed changes.