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

bpo-45637: Store the frame pointer in the cframe #29267

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Oct 28, 2021

This provides better machine level debugger support. Since the cframe is always in memory, it can be reliably found by debuggers, unlike the frame local variable which might be (ideally should be) in a register.

In theory performance might improve a bit as the cframe is on the C stack, the fastest memory to access in any system. However benchmarking results are just noise; at least it isn't obviously slower.

https://bugs.python.org/issue45637

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

This PR would break all current out-of-process debuggers and profilers. All these tools are reliying on iterating over all the active frames by accessing the current frame from the thread states that are obtained from the _PyRuntime symbol. Removing it from there will be a masive breakage for all these tools.

Edit: See new comment

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal
Copy link
Member

pablogsal commented Oct 28, 2021

Hmmm, I just saw the current frame is still accessible through the cframe pointer on the thread state. I think that is certainly better, although it complicates things still because more memory needs to be fetched (one extra level) and this memory is stack memory, which may complicate doing this from core files. I think we should leave the pointer in the thread state still as it has a negligible cost and it makes things much more simple and with less work to adapt.

In short, I still prefer this approach: #29257

@markshannon
Copy link
Member Author

markshannon commented Oct 28, 2021

Either we store a pointer to the current frame in the cframe or in tstate; not both.
Storing it in both is error prone, as they could all too easily get out of sync.

Regarding efficiency, storing it the cframe is (in theory) more efficient as the hottest accesses to the frame are in the interpreter.

@pablogsal
Copy link
Member

Regarding efficiency, storing it the cframe is (in theory) more efficient as the hottest accesses to the frame are in the interpreter.

I agree, but we are talking about not making life more difficult for other tools not about efficiency.

I think this would be workable, but I am a bit concerned about the extra hoop through the cframe. I will experiment a bit to see if this is going to be a real problem or not. For now, let's land this PR and we can revert it if it results a big problem,.

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Wait, hold on, I am cheching and this approach sadly doesn't work with threads as cframe is still optimized out in threads :(

@pablogsal
Copy link
Member

See: #29257 (comment)

@pablogsal
Copy link
Member

I think we need to just mark the test as skippable if Python is compiled with optimizations, which is the case in the ASAN build.

@markshannon
Copy link
Member Author

markshannon commented Oct 28, 2021

What does 'optimized out' mean here? cframe is in memory. It cannot be replaced by scalars.

@pablogsal
Copy link
Member

pablogsal commented Oct 28, 2021

What does 'optimized out' mean here? cframe is in memory. It cannot be replaced by scalars.

It means that gdb cannot read the value and shows it as "optimized out". This is a common problem if the DWARF data is not good enough to reconstruct whatever dance the optimizer is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants