Page MenuHomePhabricator

[ELF] Special case --shuffle-sections=-1 to reverse input sections
ClosedPublic

Authored by MaskRay on Mar 11 2021, 12:05 PM.

Details

Summary

If the number of sections changes, which is common for re-links after
incremental updates, the section order may change drastically.

Special case -1 to reverse input sections. This is a stable transform.
The section order is more resilient to incremental updates. Usually the
code issue (e.g. Static Initialization Order Fiasco, assuming pointer
comparison result of two unrelated objects) is due to the relative order
between two problematic input files A and B. Checking the regular order
and the reversed order is sufficient.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 11 2021, 12:05 PM
MaskRay requested review of this revision.Mar 11 2021, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 12:05 PM
MaskRay edited the summary of this revision. (Show Details)Mar 11 2021, 12:13 PM
MaskRay edited the summary of this revision. (Show Details)Mar 11 2021, 12:16 PM
jhenderson added inline comments.Mar 12 2021, 1:40 AM
lld/ELF/Writer.cpp
1305
lld/test/ELF/shuffle-sections.s
30

Do you need the --symbol-ordering-file option here? It seems like it's unrelated to testing whether -1 reverses the section order. If there's an interaction you want to test between the two, perhaps we should have a case with and a case without.

No fundamental objections, although I'm wondering if static constructor order is the main problem it would be better to do something specific. For example reversing the order of the constructors of the same priority while holding the remainder of the program constant. A smaller local change would help rule out other layout changes due to section order change.

Had an idle thought that it might be possible to do some kind of dependency check. For example trace the relocations from each static constructor and look for common read-write data sections. While not a guarantee of order-dependence it would be a good indicator that there may be some order dependence.

Thanks for the comments!

No fundamental objections, although I'm wondering if static constructor order is the main problem it would be better to do something specific. For example reversing the order of the constructors of the same priority while holding the remainder of the program constant. A smaller local change would help rule out other layout changes due to section order change.

Our internal system actually depends on .ctors/.dtors and hasn't migrated to .init_array/.fini_array yet ;-) Doing the migration has low priority, though.
I have thought about --shuffle-sections .init_array=-1 (<section-pattern>=<seed>) to reverse a specific set of sections.
If such an option exists, we may be able to drop our -fno-use-init-array and catch up the toolchain evolution of obsoleting .ctors/.dtors .
I do not know whether it is too specific.

In the meantime, I have filed a feature request on REVERSE(...) https://sourceware.org/bugzilla/show_bug.cgi?id=27565
If both REVERSE(...) and linker script extension (https://sourceware.org/bugzilla/show_bug.cgi?id=26404) are supported, we can actually do it with a linker script.

Had an idle thought that it might be possible to do some kind of dependency check. For example trace the relocations from each static constructor and look for common read-write data sections. While not a guarantee of order-dependence it would be a good indicator that there may be some order dependence.

Yes, some kind of dependency based checking may be useful to implement a bettern --warn-backrefs. Currently --warn-backrefs just catches problems which GNU ld/gold catch.
However, the check is performed with one particular input order so there may be other lurking problems. This is an interesting topic but I haven't thought much on it.

MaskRay marked 2 inline comments as done.Mar 15 2021, 7:37 PM
MaskRay added inline comments.
lld/test/ELF/shuffle-sections.s
30

The -1 test does not need it. I'll delete it.

--symbol-ordering-file has been tested by the lines above so the interaction does not need re-test.

MaskRay updated this revision to Diff 330862.Mar 15 2021, 7:37 PM
MaskRay marked an inline comment as done.

address comments

The pre-merge checks appear to be failing with the test.

jhenderson accepted this revision.Mar 17 2021, 1:22 AM

Functionally, the change LGTM. Please wait for @peter.smith.

This revision is now accepted and ready to land.Mar 17 2021, 1:22 AM

I'm fine with the change.