Skip to content

Affirm -Cforce-frame-pointers=off does not override #140774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Affirm -Cforce-frame-pointers=off does not override #140774

wants to merge 3 commits into from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented May 7, 2025 •
edited
Loading

This PR exists to document that we (that is, the compiler reviewer) implicitly made a decision in #86652 that defies the expectations of some programmers. Some programmers believe -Cforce-frame-pointers=false should obey the programmer in all cases, forcing the compiler to avoid generating frame pointers, even if the target specification would indicate they must be generated. However, many targets rely on frame pointers for fast or sound unwinding.

T-compiler had a weekly triage meeting on 2025-05-22. This topic was put to discussion because some programmers may expect the target-overriding behavior. In that meeting we decided removing frame pointers, at least with regards to the contract of the -Cforce-frame-pointers option, is not required, even if =off is passed, and that we will not do so if the target would expect them. This follows from the documentation here: https://doc.rust-lang.org/rustc/codegen-options/index.html#force-frame-pointers

We may separately pursue trying to clarify the situation more emphatically in our documentation, or warn when people pass the option when it doesn't do anything.

@workingjubilee workingjubilee added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiler-nominated Nominated for discussion during a compiler team meeting. labels May 7, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2025
@workingjubilee workingjubilee added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented May 7, 2025 •
edited
Loading

The current behavior of being a ratchet makes most sense to me. Enabling force-frame-pointers forces frame pointers to be used, disabling it is equivalent to not passing it at all and causes the default to be used. This matches the behavior of force-unwind-tables (for which the current behavior is required. disabling unwind tables when they are enabled by default is unsound and picking the target default even when force-unwind-tables is enabled makes the cli option useless)

@Noratrieb
Copy link
Member

I agree and think the naming conveys this as well. The absence of force should not force the opposite, it just leaves it up to the compiler to choose.

In fact, this is already clearly documented: https://doc.rust-lang.org/rustc/codegen-options/index.html?highlight=force-frame#force-frame-pointers

n, no, off or false: do not force-enable frame pointers. This does not necessarily mean frame pointers will be removed.

And this documentation has been there basically forever: #65136

@workingjubilee
Copy link
Member Author

Yes. This is essentially me looking for a clarification that yes, we intended to head in this direction, to make sure I have something authoritative to point to if it comes up again and help inform any near-future decisions about frame-pointer-related things.

@workingjubilee workingjubilee mentioned this pull request May 8, 2025
Merged
@apiraino
Copy link
Contributor

apiraino commented May 22, 2025 •
edited
Loading

Discussed durint T-compiler triage on Zulip.

We agree the situation with this multiple-value boolean flag is awkward. Maybe updating the documentation (comment) could alleviate this confusion.

We are in favor of accepting this patch! Thanks for raising the topic.

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label May 22, 2025
@apiraino apiraino added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 29, 2025
@apiraino
Copy link
Contributor

apiraino commented May 29, 2025 •
edited
Loading

I guess the next step is getting someone review this - right?

@workingjubilee please proceed with this work (if you need to) of feel free to ask for a review

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2025
@workingjubilee
codegen: test frame pointer attr prefers CLI opt
f27ed88 
This test only makes sense if you send it back in time and run it with
a now-old Rust commit, e.g. 50e0cc5
However, if you do go back that far in time, you will see it pass.
@workingjubilee workingjubilee force-pushed the should-force-frame-pointers-favor-the-target-or-cli branch from aaa10fe to e05c680 Compare June 4, 2025 21:51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee workingjubilee force-pushed the should-force-frame-pointers-favor-the-target-or-cli branch 2 times, most recently from 59c6fb8 to 4d62cab Compare June 4, 2025 22:48
@workingjubilee workingjubilee changed the title Should -Cforce-frame-pointers favor the target or CLI? Decide -Cforce-frame-pointers favors the target Jun 4, 2025
@workingjubilee workingjubilee changed the title Decide -Cforce-frame-pointers favors the target Affirm -Cforce-frame-pointers=off does not override Jun 4, 2025
@workingjubilee workingjubilee changed the title Affirm -Cforce-frame-pointers=off does not override compiler: affirm -Cforce-frame-pointers=off does not override Jun 4, 2025
@workingjubilee workingjubilee changed the title compiler: affirm -Cforce-frame-pointers=off does not override Affirm -Cforce-frame-pointers=off does not override Jun 4, 2025
@workingjubilee
Copy link
Member Author

Yes, I'm indecisive sometimes.

r? compiler

@workingjubilee workingjubilee added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2025
jieyouxu
jieyouxu approved these changes Jun 5, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one suggestion, then r=me.

tests/codegen/frame-pointer-cli-control.rs Outdated
Comment on lines 42 to 52
#![feature(no_core, lang_items)]
#![no_core]
#[lang = "sized"]
trait Sized {}
#[lang = "copy"]
trait Copy {}
impl Copy for u32 {}

// CHECK: define i32 @peach{{.*}}[[PEACH_ATTRS:\#[0-9]+]] {
#[no_mangle]
pub fn peach(x: u32) -> u32 {
x
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: maybe leave an impl comment here to say sth like

This does not use minicore to make it explicitly clear what compiler flags are involved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah actually it just doesn't use minicore because I was lazy when updating this test

@jieyouxu
Copy link
Member

jieyouxu commented Jun 5, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2025
@workingjubilee
codegen: modernize frame-pointer-cli-control.rs
a6b62d8 
Update this time-traveler on the changes in compiletest and target specs
that they missed over the pass ~3 years by being caught in a time rift.
The aarch64-apple rev splits into itself and aarch64-apple-on, because
rustc obtained support for non-leaf frame-pointers ever since 9b67cba
implemented them and used them in aarch64-apple-darwin's spec.

Note that the aarch64-apple-off revision fails, despite modernization.
This is because 9b67cba also changed the behavior of rustc to defer to
the spec over the command-line interface.
@workingjubilee
encode compiler team acceptance of -Cforce-frame-pointers change
e57b4b1
@workingjubilee workingjubilee force-pushed the should-force-frame-pointers-favor-the-target-or-cli branch from 4d62cab to e57b4b1 Compare June 5, 2025 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@workingjubilee @rust-log-analyzer @bjorn3 @Noratrieb @apiraino @jieyouxu @rustbot