Page MenuHomePhabricator

[llvm-objcopy] Fix SHT_GROUP ordering.

Authored by rupprecht on May 29 2019, 12:18 PM.



When llvm-objcopy sorts sections during finalization, it only sorts based on the offset, which can cause the group section to come after the sections it contains. This causes link failures when using gold to link objects created by llvm-objcopy.

Fix this for now by copying GNU objcopy's behavior of placing SHT_GROUP sections first. In the future, we may want to remove this sorting entirely to more closely preserve the input file layout.

This fixes

Diff Detail


Event Timeline

rupprecht created this revision.May 29 2019, 12:18 PM
MaskRay added inline comments.May 29 2019, 7:46 PM
1640 ↗(On Diff #202020)

@jakehehrlich I can't find a use site of objcopy::elf::Object::Sections that requires it to be sorted.

I guess we can probably just delete this function and fix the 5 or 6 tests that check the section order.

MaskRay added inline comments.May 29 2019, 7:48 PM
1643 ↗(On Diff #202020)

This also matches what GNU objcopy does.

This is not accurate.. I suspect it just doesn't sort sections, unless in --pad-to= or --gap-fill= mode.

MaskRay added inline comments.May 29 2019, 9:56 PM
1 ↗(On Diff #202020)

Regarding this test, I'm not sure if llvm-objcopy should repair the object file. It seems currently the only broken tool is us :( After this issue is fixed, the repair functionality may be use useful any more...

1643 ↗(On Diff #202020)

Sorry, GNU objcopy does move SHT_GROUP sections before.

bfd/elf.c:assign_section_numbers does this. It has a comment:

/* Put SHT_GROUP sections first. */

Can we just sort in the layout code instead of sorting everything? Then we don't have to sort at all. I think we've largely been good about making operations stable so order should be maintained if just don't sort.

1640 ↗(On Diff #202020)

The layout algorithm assumes that things are sorted by offset so we can't just delete this but we can sort a copy there instead.

Alternatively we can sort by Index to allow ourselves to make mistakes in maintaining the order along the way but I think I'd rather not do that since that's just a hack.

Alternatively we could only sort program headers in the layout algorithm and then just layout sections in whatever order we please. It will just mean that our layout won't mirror the input but it will be valid.

rupprecht marked 5 inline comments as done.Jun 5 2019, 6:02 PM
rupprecht added inline comments.
1 ↗(On Diff #202020)

Mind clarify what's broken in the object file? The input here is valid -- .group contains .bar, and comes before it -- but the test shows that .group moves to index 1.

It may also repair a broken object file, but I didn't test if it does that. But this should fix a bug by not *creating* a broken object file.

1640 ↗(On Diff #202020)

I don't understand this either. I need to study it in more detail, but Object::layoutSections iterates over sections (in the order as sorted here) and assigns offsets based on what the aligned value of (previous offset + previous size) is... that doesn't require them to be sorted, AFAICT.

1643 ↗(On Diff #202020)

Correct; sections are rearranged (for SHT_GROUP) but not sorted.

MaskRay accepted this revision.Jun 6 2019, 11:06 PM

I don't have more comments. This does make llvm-objcopy behave more like GNU objcopy and fixes the SHT_GROUP bug. Such object files are accepted by ld.bfd and lld as a natural extension, but gold takes another approach to process SHT_GROUP and would give an error.

I slightly prefer deleting Object::sortSections() (I can't find anywhere the sorted behavior is required by the layout algorithm) but the current change is also good.

1 ↗(On Diff #202020)

Sorry you can ignore my previous comment. I made the comment before realizing sorting is the behavior of GNU objcopy.

This revision is now accepted and ready to land.Jun 6 2019, 11:06 PM
This revision was automatically updated to reflect the committed changes.
rupprecht marked an inline comment as done.