This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Make SORT_INIT_PRIORITY support .ctors.N
ClosedPublic

Authored by MaskRay on Nov 10 2020, 10:24 AM.

Details

Summary

Input sections .ctors/.ctors.N may go to either the output section .init_array or the output section .ctors:

  • output .ctors: currently we sort them by name. This patch changes to sort by priority from high to low. If N in .ctors.N is in the form of %05u, there is no semantic difference. Actually GCC and Clang do use %05u. (In the test ctors_dtors_priority.s and Gold's test gold/testsuite/script_test_14.s, we can see %03u, but they are not really produced by compilers.)
  • output .init_array: users can provide an input section description SORT_BY_INIT_PRIORITY(.init_array.* .ctors.*) to mix .init_array.* and .ctors.*. This can make .init_array.N and .ctors.(65535-N) interchangeable.

With this change, users can mix .ctors.N and .init_array.N in .init_array (PR44698 and PR48096) with linker scripts. As an example:

SECTIONS {
  .init_array : {
    *(SORT_BY_INIT_PRIORITY(.init_array.* .ctors.*))
    *(.init_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .ctors)
  }
} INSERT AFTER .fini_array;
SECTIONS {
  .fini_array : {
    *(SORT_BY_INIT_PRIORITY(.fini_array.* .dtors.*))
    *(.fini_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .dtors)
  }
} INSERT BEFORE .init_array;

Diff Detail

Event Timeline

MaskRay created this revision.Nov 10 2020, 10:24 AM
MaskRay requested review of this revision.Nov 10 2020, 10:24 AM

Apologies for being so quiet recently. It looks like this is changing .ctors/.dtors away from SORT_BY_NAME when they aren't mixed with .init_array.

lld/ELF/OutputSections.cpp
477

I'm not sure this is right. As I understand it compCtors will only be called when not using a linker script.

In that case we'd have a separate .ctors and .dtors section to an existing .init_array section and in this case the rules for sorting ctors and dtors are different i.e. the default is sort by name (see comment above). It looks like this bit of code will change the default linker script to the equivalent of:

.ctors : { SORT_BY_INIT_PRIORITY (*(.ctors)) }

from:

.ctors : { SORT_BY_NAME (*(.ctors)) }

I think the old way was closer to BFD as that only changes to SORT_BY_INIT_PRIORITY if .ctors are placed in the .init_array OutputSection.

MaskRay added a comment.EditedNov 10 2020, 2:28 PM

Good to hear from you:) Thanks for review!

For GNU ld's internal linker script, the effective part for sections is:

  .init_array    :
  {
    KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*) SORT_BY_INIT_PRIORITY(.ctors.*)))
    KEEP (*(.init_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .ctors))
  }
  .ctors          :
  {
    KEEP (*crtbegin.o(.ctors))   /* Get a -1 sentinel from crtbegin*.o */
    KEEP (*crtbegin?.o(.ctors))
/* the sort-by-name pattern is ineffective due to the pattern in .init_array */
/* the body is essentially empty */
    KEEP (*crtend.o(.ctors) *crtend?.o(.ctors)) /* Get a 0 sentinel from crtend*.o */
  }

You are right that there is a semantic difference.

Clang emits .ctors.%05u and GCC seems to emit .ctors.%.5u. Both should work fine with either SORT (by name, i.e. string comparison, "100" < "5") and SORT_BY_INIT_PRIORITY (by priority). So the change should matter in practice. Note that .ctors.100 has already been converted to .init_array in GNU ld by priority while with the output section .ctors, they should be sorted by name. So I think this semantic change does not matter.

(GCC does not have any test for .ctors.N ; GNU ld only has tests for .ctors.N where N is in the %05u form. .ctors.101 exists in a gold test but it does not seem to match the practical case)

MaskRay added inline comments.Nov 10 2020, 3:02 PM
lld/ELF/OutputSections.cpp
477

The idea is: getPriority(a->name) > getPriority(b->name) is equivalent to a->name < b->name for .ctors.N sections where N is %05u

If the output section is .ctors, the order of input sections: .ctors, .ctors.00005, .ctors.00100
(This is tested by ctors_dtors_priority.s)

MaskRay edited the summary of this revision. (Show Details)Nov 10 2020, 10:04 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited the summary of this revision. (Show Details)Nov 10 2020, 10:07 PM

Will take another look tonight.For reference, I tried helping out someone last year with https://bugs.llvm.org/show_bug.cgi?id=44698 I think they had old objects, or a an old GCC on their system and it was mixing with a modern clang. Maybe worth a read through.

Will take another look tonight.For reference, I tried helping out someone last year with https://bugs.llvm.org/show_bug.cgi?id=44698 I think they had old objects, or a an old GCC on their system and it was mixing with a modern clang. Maybe worth a read through.

Thanks for the pointer. (I did recall we had another bug about .ctors (which motivated me to investigate the behaviors and observe several problems) but did not find the bug number..

With this patch, the user can provide a linker script fragment

SECTIONS {
  .init_array : {
    *(SORT_BY_INIT_PRIORITY(.init_array.* .ctors.*))
    *(.init_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .ctors)
  }
} INSERT AFTER .fini_array;
SECTIONS {
  .fini_array : {
    *(SORT_BY_INIT_PRIORITY(.fini_array.* .dtors.*))
    *(.fini_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .dtors)
  }
} INSERT BEFORE .init_array;

The use of INSERT [AFTER|BEFORE] is a bit of an abuse but in the absence of a better syntax (https://sourceware.org/bugzilla/show_bug.cgi?id=26404) this may be the best the user can achieve without providing a full script.

MaskRay updated this revision to Diff 304611.Nov 11 2020, 11:48 AM

The original comment was wrong about point 2 and 3. Rewrite the comment.

MaskRay updated this revision to Diff 304613.Nov 11 2020, 11:50 AM

Fix potential use after free

MaskRay marked an inline comment as done.Nov 11 2020, 11:52 AM
MaskRay edited the summary of this revision. (Show Details)Nov 11 2020, 11:59 AM
psmith accepted this revision.Nov 12 2020, 3:59 AM

Thanks for updating the comments. If the lexical ordering is going to work out the same I don't have any objections.

This revision is now accepted and ready to land.Nov 12 2020, 3:59 AM
This revision was automatically updated to reflect the committed changes.