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

Set non-leaf frame pointers on Fuchsia targets #124677

Merged
merged 1 commit into from
May 4, 2024

Conversation

djkoloski
Copy link
Contributor

@djkoloski djkoloski commented May 3, 2024

This is part of our work to enable shadow call stack sanitization on Fuchsia, see this Fuchsia issue.

r? @tmandry

@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 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@tmandry
Copy link
Member

tmandry commented May 3, 2024

@djkoloski Is there any motivation/context you can link to?

I assume this has to do with the SCS work.

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2024

📌 Commit 0637709 has been approved by tmandry

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2024
@djkoloski
Copy link
Contributor Author

Yeah it's SCS related. I updated the PR description with a link to Fuchsia issue.

@ilovepi
Copy link
Contributor

ilovepi commented May 3, 2024

@tmandry We plan to enable Non-Leaf frame pointers everywhere as part of Fuchsia's default options. After a thorough evaluation, we found that across most workloads in Fuchsia, there is no significant performance impact when enabling frame pointers w/ -momit-leaf-frame-pointer or the Rust equivalent of NonLeaf. The benchmarking held even for Google workloads, and open source benchmark suites, like fleetbench, eigen, and chrome benchmarks, across both X86_64 and Aarch64 CPUs.

Ideally, setting NonLeaf would be controllable through a compiler option though, and wouldn't be something you can only set in spec/base.

This isn't purely for SCS, though the motivation is the same: provide a uniform method for fast unwinding on Fuchsia.

@tmandry
Copy link
Member

tmandry commented May 3, 2024

Thanks for the context @ilovepi.

@workingjubilee
Copy link
Member

I was actually evaluating various platforms for this recently and I felt I should ask: is Fuchsia's aarch64 implementation supposed to follow the AAPCS64 ABI, or is that a "wrong question"?

@ilovepi That's good to know! Is there a convenient way for someone outside Google to view a quantified form of "no significant performance impact"? "Somewhat inconvenient, actually" is also good, really, as long as it's reachable by, uh, me.

@ilovepi
Copy link
Contributor

ilovepi commented May 3, 2024

I was actually evaluating various platforms for this recently and I felt I should ask: is Fuchsia's aarch64 implementation supposed to follow the AAPCS64 ABI, or is that a "wrong question"?

@ilovepi That's good to know! Is there a convenient way for someone outside Google to view a quantified form of "no significant performance impact"? "Somewhat inconvenient, actually" is also good, really, as long as it's reachable by, uh, me.

I'm not sure I have any documents handy that I can share externally, but benchmarking even the llvm-test suite w/ -momit-leaf over 10 runs was hard to distinguish from the noise. We ran benchmarks for a long time internally though, but even that simple experiment should give you a bit of confidence that there is a decent amount of variance in the benchmarks, even once you try to account for voltage scaling, and all the other gotchas that come w/ benchmarking. Also, FYI, there isn't much that's internal in the document or findings, I just don't think its going to be easy to share.

I've posted here what I think the take aways should be, but I did use internal benchmarking facilities to get real perf numbers using Google infrastructure, including running open source benchmarks. As a point of reference, Google enables -fno-omit-frame-pointer -momit-leaf-frame-poiner on every binary, including search, Ads, YouTube. This isn't secret sauce though, IIRC Meta, and Apple (and many other tech companies) do the same.

@workingjubilee
Copy link
Member

Thanks! I know the numbers tend to line up for most of these cases, I was just hoping for a giant slab of collected numbers to bury people with discuss. :^) I'll get them someday.

@workingjubilee workingjubilee added the O-fuchsia Operating system: Fuchsia label May 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 4, 2024
…r, r=tmandry

Set non-leaf frame pointers on Fuchsia targets

This is part of our work to enable shadow call stack sanitization on Fuchsia, see [this Fuchsia issue](https://g-issues.fuchsia.dev/issues/327643884).

r? `@tmandry`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#123356 (Reduce code size of `thread::set_current`)
 - rust-lang#124159 (Move thread parking to `sys::sync`)
 - rust-lang#124293 (Let miri and const eval execute intrinsics' fallback bodies)
 - rust-lang#124500 (lldb-formatters: Use StdSliceSyntheticProvider for &str)
 - rust-lang#124677 (Set non-leaf frame pointers on Fuchsia targets)
 - rust-lang#124692 (We do not coerce `&mut &mut T -> *mut mut T`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#123356 (Reduce code size of `thread::set_current`)
 - rust-lang#124159 (Move thread parking to `sys::sync`)
 - rust-lang#124293 (Let miri and const eval execute intrinsics' fallback bodies)
 - rust-lang#124677 (Set non-leaf frame pointers on Fuchsia targets)
 - rust-lang#124692 (We do not coerce `&mut &mut T -> *mut mut T`)
 - rust-lang#124698 (Rewrite `rustdoc-determinism` test in Rust)
 - rust-lang#124700 (Remove an unnecessary cast)
 - rust-lang#124701 (Docs: suggest `uN::checked_sub` instead of check-then-unchecked)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 81b43b7 into rust-lang:master May 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
Rollup merge of rust-lang#124677 - djkoloski:set_fuchsia_frame_pointer, r=tmandry

Set non-leaf frame pointers on Fuchsia targets

This is part of our work to enable shadow call stack sanitization on Fuchsia, see [this Fuchsia issue](https://g-issues.fuchsia.dev/issues/327643884).

r? ``@tmandry``
@ilovepi
Copy link
Contributor

ilovepi commented May 6, 2024

@workingjubilee I missed the AAPCS64 ABI part of your question. As far as I know, we are. https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#the-frame-pointer spells out when leaf frames can be omitted from the frame pointer chain, which matches typical usage on other platforms. The other ABI parts that I'm aware of for Aarch64, usage of platform register and calling conventions, also comply with that document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-fuchsia Operating system: Fuchsia S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants