GH-126910: avoid reading the FP for getting the SP#146521
Merged
Fidget-Spinner merged 2 commits intopython:mainfrom Mar 27, 2026
Merged
GH-126910: avoid reading the FP for getting the SP#146521Fidget-Spinner merged 2 commits intopython:mainfrom
Fidget-Spinner merged 2 commits intopython:mainfrom
Conversation
Fidget-Spinner
approved these changes
Mar 27, 2026
Member
There was a problem hiding this comment.
To anyone reading this:
The TLDR it seems is that __builtin_frame_address forces the frame pointer to be materialized per-function as it requires reading the FP! This is obviously bad, as that means we get the FP prologue on every function that has this, which corrupts the real FP in the JIT code (the one from the shim).
Diego and I spent 1 hour pairing to find out this problem only by analyzing the generated asm. I am going to write a lightweight assembly verifier to make sure we don't regress on this and guarantee that don't corrupt the FP on the JIT anymore. This also ensures unwinders using FP will always properly work with the JIT.
Member
|
@Fidget-Spinner, would marking the function that calls the intrinsic as “noinline” work? IIUC the issue is that we’re inlining the intrinsic into JIT code. |
Member
I'm inclined to not do that, as the JIT stencils will need to point to another extern call which will slow down this supposedly cheap check. |
Member
|
@brandtbucher it seems you're right and we need to @diegorusso This is the patch I have |
|
Member
|
What was wrong with the first patch? |
Member
|
@markshannon the first solution didnt pass CI. I suspect when the first solution was inlined, it gave the wrong value altogether. |
Member
|
Did you look at why the tests failed? To me, that failure looks like the UB sanitizer using too much stack. |
Member
Yeah it looked like that to me as well, but the UB sanitizer depends on an semi-accurate value of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is needed for #146071