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

[Core 12] arch/amd64: let the frame pointer return #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MingcongBai
Copy link
Member

Apologies for abruptly reverting that change but this change is best queued for Core 12 when we survey potential changes for core toolchains. cc @Artoria2e5 @jiegec

From original pull request:

https://www.brendangregg.com/blog/2024-03-17/the-return-of-the-frame-pointers.html argues that amd64 has enough registers to not be bothered by the FP. Google, Meta, Fedora, Ubuntu, and Arch seem to agree.

Should we follow suit? The main justification is that the FP gives real-time profiling tools a proper view of the stack.

Artoria2e5 and others added 2 commits June 18, 2024 14:09
https://www.brendangregg.com/blog/2024-03-17/the-return-of-the-frame-pointers.html
argues that amd64 has enough registers to not be bothered by the FP.
Google, Meta, Fedora, Ubuntu, and Arch seem to agree.

This commit affects all arches that do not override the following
variables:

* CFLAGS_COMMON_OPTI
* RUSTFLAGS_COMMON_OPTI
@MingcongBai MingcongBai added the enhancement New feature or request label Jun 18, 2024
@MingcongBai
Copy link
Member Author

And, for the record, I second this change.

@Artoria2e5
Copy link
Member

Artoria2e5 commented Jun 18, 2024

Huh, I wasn't even aware that the original got merged. Must have slipped my mind…

@MingcongBai
Copy link
Member Author

Huh, I wasn't even aware that the original got merged. Must have slipped my mind…

It was a gap in communication - I have noted that we should wait for Core 12 (i.e., in a month) before this change but only on Telegram. Sorry about that.

@@ -4,7 +4,7 @@

# C Compiler Flags.
CFLAGS_COMMON=('-pipe' '-Wno-error')
CFLAGS_COMMON_OPTI=('-O2')
CFLAGS_COMMON_OPTI=('-O2' '-fno-omit-frame-pointer' '-mno-omit-leaf-frame-pointer')
Copy link
Member

Choose a reason for hiding this comment

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

Some CPU architectures may not have the concept of "frame pointers", enabling frame pointers on those platforms will cause the compiler to use an emulated version of "frame pointers" (while sacrificing an extra register for storing emulated frame pointer data).

Copy link
Member

@Artoria2e5 Artoria2e5 Oct 29, 2024

Choose a reason for hiding this comment

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

This is the case for almost any architecture. The concept of a frame pointer comes from the ABI, not from the architecture. In the case of x86-64, %rbp is one of 16 GP registers chosen to act as a frame pointer by ABI convention, and using -fomit frees it to be used for other purposes.

Now if 16 GP registers is enough to not omit the one frame pointer, the many RISC architectures with 32 or more GP registers must be completely okay with not omitting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants