Page MenuHomePhabricator

[TargetLoweringObjectFileImpl] Produce .text.hot. instead of .text.hot for -fno-unique-section-names
ClosedPublic

Authored by MaskRay on May 7 2020, 2:21 PM.

Details

Summary

GNU ld's internal linker script uses (https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=add44f8d5c5c05e08b11e033127a744d61c26aee)

.text           :
{
  *(.text.unlikely .text.*_unlikely .text.unlikely.*)
  *(.text.exit .text.exit.*)
  *(.text.startup .text.startup.*)
  *(.text.hot .text.hot.*)
  *(SORT(.text.sorted.*))
  *(.text .stub .text.* .gnu.linkonce.t.*)
  /* .gnu.warning sections are handled specially by elf.em.  */
  *(.gnu.warning)
}

Because *(.text.exit .text.exit.*) is ordered before *(.text .text.*), in a -ffunction-sections build, the C library function exit will be placed before other functions.
gold's -z keep-text-section-prefix has the same problem.

In lld, -z keep-text-section-prefix recognizes .text.{exit,hot,startup,unlikely,unknown}.*, but not .text.{exit,hot,startup,unlikely,unknown}, to avoid the strange placement problem.

In -fno-function-sections or -fno-unique-section-names mode, a function whose function_section_prefix is set to .exit"
will go to the output section .text instead of .text.exit when linked by lld.
To address the problem, append a dot to become .text.exit.

Diff Detail

Event Timeline

MaskRay created this revision.May 7 2020, 2:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 2:21 PM
MaskRay edited the summary of this revision. (Show Details)May 7 2020, 2:22 PM
MaskRay edited the summary of this revision. (Show Details)May 7 2020, 3:27 PM
MaskRay edited the summary of this revision. (Show Details)May 7 2020, 3:37 PM
wmi added a comment.May 7 2020, 4:13 PM

Although less likely, is there possiblity that someone has already been using attribute((section(".text.hot."))? Adding another trailing period will break the existing usage. We may either tolerate that case, or trigger an assertion.

In D79600#2026282, @wmi wrote:

Although less likely, is there possiblity that someone has already been using attribute((section(".text.hot."))? Adding another trailing period will break the existing usage. We may either tolerate that case, or trigger an assertion.

__attribute__((section(".text.hot.")) overrides the default section name .text.$function or .text.$prefix.$function

__attribute__((section(".text.hot."))) int foo() { return 42; } produces .text.hot. with or without -ffunction-sections

@MaskRay Could you please elaborate more on the problem being solved as it is not still fully clear to me? Is it that only the "exit" function has this problem? If you could also mention why the linker is not the right place to handle this problem, it would be useful. Thanks.

@MaskRay Could you please elaborate more on the problem being solved as it is not still fully clear to me? Is it that only the "exit" function has this problem? If you could also mention why the linker is not the right place to handle this problem, it would be useful. Thanks.

GNU ld's internal linker script orders *(.text.exit .text.exit.*) before *(.text .text.*). This property makes C functions unlikely exit hot startup to be ordered before other normal C functions in a -ffunction-sections build.

So in lld, I don't want to let -z keep-text-section-prefix recognize .text.exit (and the like). lld should just recognize .text.exit.*

Because of this, in -fno-unique-section-names mode, we may get .text.exit which will not go to the output section .text.exit. To fix that, name the input section .text.exit.

MaskRay edited the summary of this revision. (Show Details)May 8 2020, 12:03 PM

As far as I can tell, this LGTM. I am not an expert on section names and how this affects assumptions in GNU land, so please do seek an expert's review if necessary.

As far as I can tell, this LGTM. I am not an expert on section names and how this affects assumptions in GNU land, so please do seek an expert's review if necessary.

.text.exit. works with GNU ld's internal linker script and gold's -z keep-text-section-prefix. I CCed some linker experts.

grimar accepted this revision.May 12 2020, 4:02 AM

After reading the discussion in D79590, I think this makes sense, so LGTM. Please hold this a bit, perhaps Peter or somebody else might also have comments.

This revision is now accepted and ready to land.May 12 2020, 4:02 AM

I don't have a lot to add. I think this change will work with LLD and the GNU tools, no objections to this change. Having LLD not recognise the section names without a suffix may become a problem if GNU tools still output them. My guess is that LLD is still most used with clang at the moment, but this may change over time.

MaskRay added a comment.EditedMay 12 2020, 8:43 AM

I don't have a lot to add. I think this change will work with LLD and the GNU tools, no objections to this change. Having LLD not recognise the section names without a suffix may become a problem if GNU tools still output them. My guess is that LLD is still most used with clang at the moment, but this may change over time.

Yes, this will work with lld, GNU ld and gold. Your concern may be that GCC will still emit .text.exit? It emits .text.exit.* (function section manner). It does not support -fno-unique-section-names. Part of the reasons is that the related GNU as feature is a fairly new addition 2020-02 http://sourceware.org/PR25380 (I am going to raise a GCC feature request.)

edit: filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95095

This revision was automatically updated to reflect the committed changes.