Skip to content
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.

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

Add support for -Cforce-frame-poiners=non-leaf #125127

Closed
wants to merge 2 commits into from

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented May 14, 2024

This brings the command-line option up to parity with the frame-pointers field in the json target specs. It's still backwards compatible with before but now also accepts always, never and non-leaf.

@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@jieyouxu
Copy link
Member

jieyouxu commented May 14, 2024

Doesn't #124733 try to do this already?

This brings the command-line option up to parity with the frame-pointers
field in the json target specs.
@jsgf jsgf force-pushed the frame-pointers-non-leaf branch 2 times, most recently from 3be218c to 3112454 Compare May 14, 2024 18:31
@workingjubilee
Copy link
Member

Yes, I'm sorry @jsgf but I believe my PR already completely supersedes yours, which would require all the additional bits that mine handles (namely, landing behind -Zunstable-options and also Doing A Bureaucracy).

@workingjubilee
Copy link
Member

workingjubilee commented May 14, 2024

I suppose I didn't add the "always" option, I would be happy to include that.

@jsgf jsgf force-pushed the frame-pointers-non-leaf branch from 3112454 to 49ea4e2 Compare May 14, 2024 18:45
@workingjubilee
Copy link
Member

workingjubilee commented May 14, 2024

I believe the "never" option is confusing as it suggests we will not force frame pointers or that we will force-them-to-not-occur, but we do demand frame pointers from the backend when it is required for ABI compliance, and not allowing people to turn this off is quite intentional as I understand it, as then it breaks important elements of runtime support.

@jsgf
Copy link
Contributor Author

jsgf commented May 14, 2024

@workingjubilee @jieyouxu Oh yeah, looks like it. I guess I should have done a search. Like you I was surprised this wasn't already an option.

@jsgf jsgf closed this May 14, 2024
@jsgf
Copy link
Contributor Author

jsgf commented May 14, 2024

@workingjubilee

I believe the "never" option

Yeah I was in two minds about this - the alternative is to just reflect the "MayOmit" terminology from the enum

fp = FramePointer::Always;
match (opts.unstable_opts.instrument_mcount, opts.cg.force_frame_pointers) {
(true, _) => fp = FramePointer::Always,
(_, Some(fp_opt)) => fp = fp_opt,
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behavior. The code did not previously override the value of fp if force_frame_pointers was Some(false).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants