naked functions: respect function-sections#147811
naked functions: respect function-sections#147811folkertdev wants to merge 3 commits intorust-lang:mainfrom
function-sections#147811Conversation
|
Some changes occurred in compiler/rustc_codegen_ssa |
rustbot
added
S-waiting-on-review
folkertdev
changed the title
function-sections
folkertdev
force-pushed
the
naked-function-sections
branch
from
2ebd197 to
ac4b179
Compare
folkertdev
force-pushed
the
naked-function-sections
branch
from
ac4b179 to
6580a5f
Compare
| if let Some(section) = &link_section { | ||
| writeln!(begin, ".pushsection {section},\"xr\"").unwrap() | ||
| } else if function_sections { | ||
| writeln!(begin, ".pushsection .text${asm_name},\"xr\"").unwrap() |
There was a problem hiding this comment.
On -msvc targets, function_sections is ignored. .text$sym is only used on -gnu targets.
There was a problem hiding this comment.
Also LLVM currently generates the following line on COFF targets:
.section {section},"xr",one_only,{sym},unique,0`
Should we be doing the same here?
There was a problem hiding this comment.
On -msvc targets, function_sections is ignored.
Meaning that it's just always assumed to be on? https://godbolt.org/z/a6ofW49YK
.text$symis only used on -gnu targets.
It should still work for msvc, no?
Should we be doing the same here?
I don't think we can reliably emit the unique,id bit of that line because how do we make that ID?
There was a problem hiding this comment.
Actually using -Zfunction-sections=no on msvc does have an effect. So I'm not really sure what it being ignored means then. Maybe this is a more recent LLVM change?
There was a problem hiding this comment.
No, it's always assumed to be off on msvc. You can see it always uses .text.
There was a problem hiding this comment.
It does always use .text but normally it emits the line you quoted:
.section .text,"xr",one_only,foo,unique,0
i.e. it uses a subsection which, as far as I understand, does allow DCE, with -Zfunction-sections=no it emits just
.text
So then DCE is impossible
There was a problem hiding this comment.
The unique,0 bit is apparently an LLVM extension https://llvm.org/docs/Extensions.html#id2. It is documented as elf-specific, but clearly it's used for COFF as well...
I just don't see how we can generate the unique ID in a reliable way. Also because we would generate a function per section, the function name (which should be unique?) should be sufficient to disambiguate the sections.
In short, I think the implementation in this PR is the best we can reliably do.
|
☔ The latest upstream changes (presumably #148305) made this pull request unmergeable. Please resolve the merge conflicts. |
folkertdev
force-pushed
the
naked-function-sections
branch
from
6580a5f to
b12830c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like windows will also be using function sections by default soon #148669 |
For `gnu` function_sections is off by default.
folkertdev
force-pushed
the
naked-function-sections
branch
from
b12830c to
77de006
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This is unlikely to happen, as you can see crashes in failed tests in that PR. That is what I was talking about in #147789 (comment) |
|
Hi @folkertdev, are you waiting for another review or is the discussion thread here still active? |
|
In my mind this was blocked on review, but now that I read it again I should give this a once-over and probably test both with and without @rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
8 participants
fixes #147789
r? @Amanieu