bpo-45637: Remove broken fallback in gdb helpers to obtain frame variable #29257

pablogsal · 2021-10-27T23:31:08Z

https://bugs.python.org/issue45637

pablogsal · 2021-10-27T23:33:26Z

This will not fix the buildbot, but at least won't give incorrect results. We need to make sure that we bail out if the frame is not optimized, but that must be done separately

pablogsal · 2021-10-27T23:34:28Z

@markshannon We should maybe place the current frame in some place that we know is not going to get optimized so we can retrieve it

markshannon · 2021-10-28T09:47:44Z

@markshannon We should maybe place the current frame in some place that we know is not going to get optimized so we can retrieve it

Yes, let's do that.

markshannon · 2021-10-28T11:18:32Z

But let's do it properly, and not in this PR. Could we restrict this PR to fixing gdb support, thanks.

If we move the frame to the cframe, then it should be removed from the thread state.
I'm benchmarking a PR to do that now.

pablogsal · 2021-10-28T12:19:59Z

But let's do it properly, and not in this PR. Could we restrict this PR to fixing gdb support, thanks.

If we move the frame to the cframe, then it should be removed from the thread state. I'm benchmarking a PR to do that now.

The current frame in the thread state is what makes external inspection tools work. Of you remove it from there you make it impossible to fetch the current frame from the _PyRuntime structure. That's a no go

Or are you talking about removing cframe from the thread state?

pablogsal · 2021-10-28T12:21:24Z

I think this approach is the best. We cannot just remove the bad gdb part because other tests rely on it unfortunately, so we are currently in this half broken state.

Adding a redundant current frame to the CFrame is a negligible cost for having the GDB bindings work on all these cases

pablogsal · 2021-10-28T12:35:28Z

For some reason, removing the fallback this breaks test_threads in the address sanitizer:

======================================================================
FAIL: test_threads (test.test_gdb.PyBtTests)
Verify that "py-bt" indicates threads that are waiting for the GIL
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_gdb.py", line 851, in test_threads
    self.assertIn('Waiting for the GIL', gdb_output)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'Waiting for the GIL' not found in 'Breakpoint 1 at 0x744390: file Python/bltinmodule.c, line 1196.\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".\n[New Thread 0x7ffff40dc700 (LWP 30738)]\n[New Thread 0x7ffff30c5700 (LWP 30739)]\n[New Thread 0x7ffff20ae700 (LWP 30740)]\n[New Thread 0x7ffff1097700 (LWP 30741)]\n\nThread 1 "python" hit Breakpoint 1, builtin_id (self=, v=42) at Python/bltinmodule.c:1196\n1196\t{\n\nThread 5 (Thread 0x7ffff1097700 (LWP 30741) "python"):\n'

----------------------------------------------------------------------

pablogsal · 2021-10-28T12:57:56Z

Let's land #29267 and then I can remove my changes for the cframe from here

…able

pablogsal · 2021-10-28T12:59:40Z

@markshannon Rebased, please review again

markshannon · 2021-10-28T13:09:11Z

Because cframe is a struct on the C stack, even if the frame-pointer is omitted, it still accessible at a fixed offset from the stack-pointer. I don't know if dwarf supports offsets from rsp, but if it does it, then (in theory) cframe should never be "optimized out", even in optimized builds.

@pablogsal does that sound right to you?

markshannon · 2021-10-28T13:10:38Z

Tools/gdb/libpython.py

+            if cframe is None:
+                return None
+            frame = PyFramePtr(cframe["current_frame"].dereference())
+            if frame and not frame.is_optimized_out():


Shouldn't it be impossible for frame to be "optimized out" here?

Not sure if impossible, because optimized out is a value that GDB cannot read, but I agree is certainly very likely never going to be True but I think is still good for safety. You want to remove the check?

Up to you. I'm just guessing here, really.

pablogsal · 2021-10-28T13:36:56Z

Because cframe is a struct on the C stack, even if the frame-pointer is omitted, it still accessible at a fixed offset from the stack-pointer. I don't know if dwarf supports offsets from rsp, but if it does it, then (in theory) cframe should never be "optimized out", even in optimized builds.

@pablogsal does that sound right to you?

That's sadly not true, is optimized out in some of the builds with -O2/O3, including the address sanitizer builds:

❯ gdb --args ./python example.py
(gdb) b builtin_id
Breakpoint 1 at 0x7062c0: file Python/bltinmodule.c, line 1197.
(gdb) r
(gdb)
#4  0x00005555556b839e in _PyEval_EvalFrameDefault (tstate=<optimized out>,
(gdb) p cframe
$1 = <optimized out>
(gdb) p frame
$2 = <optimized out>

pablogsal · 2021-10-28T13:44:08Z

@markshannon I have tried with several gcc/gdb versions and all have the same behaviour I showed in the previous comment once you use -O3 and most of them with -O2.

markshannon · 2021-10-28T14:10:09Z

@markshannon I have tried with several gcc/gdb versions and all have the same behaviour I showed in the previous comment once you use -O3 and most of them with -O2.

Oh well. Let's just do the best we can, then.

pablogsal · 2021-10-28T14:25:03Z

Oh well. Let's just do the best we can, then.

We could mark stuff as volatile but since we have the frame in the current cframe this is going to possible affect performance. Basically, we would need a redundant local that is marked as volatile or that is surrounded with pragmas for -O0.

pablogsal · 2021-10-28T14:25:53Z

@markshannon I am not super fond of this idea, but I am curious to know what you think

…able (pythonGH-29257)

pablogsal requested a review from markshannon October 27, 2021 23:31

the-knights-who-say-ni added the CLA signed label Oct 27, 2021

bedevere-bot added the awaiting core review label Oct 27, 2021

pablogsal added the skip news label Oct 27, 2021

pablogsal force-pushed the bpo-45637 branch from b9853a5 to 4289051 Compare October 28, 2021 10:43

pablogsal mentioned this pull request Oct 28, 2021

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

Merged

pablogsal force-pushed the bpo-45637 branch from 4289051 to 5db258f Compare October 28, 2021 12:55

pablogsal force-pushed the bpo-45637 branch from 5db258f to c41796e Compare October 28, 2021 12:58

bpo-45637: Remove broken fallback in gdb helpers to obtain frame vari…

850219d

…able

pablogsal force-pushed the bpo-45637 branch from c41796e to 850219d Compare October 28, 2021 12:59

markshannon reviewed Oct 28, 2021

View reviewed changes

pablogsal requested a review from markshannon October 31, 2021 22:23

pablogsal merged commit f4c0348 into python:main Nov 9, 2021

pablogsal deleted the bpo-45637 branch November 9, 2021 11:19

bedevere-bot removed the awaiting core review label Nov 9, 2021

thatbirdguythatuknownot added a commit to thatbirdguythatuknownot/cpython that referenced this pull request Nov 9, 2021

bpo-45637: Remove broken fallback in gdb helpers to obtain frame vari…

a1625e4

…able (pythonGH-29257)

thatbirdguythatuknownot added a commit to thatbirdguythatuknownot/cpython that referenced this pull request Nov 9, 2021

bpo-45637: Remove broken fallback in gdb helpers to obtain frame vari…

ba847f6

…able (pythonGH-29257)

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021

bpo-45637: Remove broken fallback in gdb helpers to obtain frame vari…

9d1695c

…able (pythonGH-29257)

remykarem pushed a commit to remykarem/cpython that referenced this pull request Jan 30, 2022

bpo-45637: Remove broken fallback in gdb helpers to obtain frame vari…

fa00196

…able (pythonGH-29257)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-45637: Remove broken fallback in gdb helpers to obtain frame variable #29257

bpo-45637: Remove broken fallback in gdb helpers to obtain frame variable #29257

pablogsal commented Oct 27, 2021 •

edited by bedevere-bot

Loading

pablogsal commented Oct 27, 2021

pablogsal commented Oct 27, 2021

markshannon commented Oct 28, 2021

markshannon commented Oct 28, 2021

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021

pablogsal commented Oct 28, 2021

markshannon commented Oct 28, 2021

markshannon Oct 28, 2021

pablogsal Oct 28, 2021 •

edited

Loading

markshannon Oct 28, 2021

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021

markshannon commented Oct 28, 2021

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021

bpo-45637: Remove broken fallback in gdb helpers to obtain frame variable #29257

bpo-45637: Remove broken fallback in gdb helpers to obtain frame variable #29257

Conversation

pablogsal commented Oct 27, 2021 • edited by bedevere-bot Loading

pablogsal commented Oct 27, 2021

pablogsal commented Oct 27, 2021

markshannon commented Oct 28, 2021

markshannon commented Oct 28, 2021

pablogsal commented Oct 28, 2021 • edited Loading

pablogsal commented Oct 28, 2021

pablogsal commented Oct 28, 2021 • edited Loading

pablogsal commented Oct 28, 2021

pablogsal commented Oct 28, 2021

markshannon commented Oct 28, 2021

markshannon Oct 28, 2021

Choose a reason for hiding this comment

pablogsal Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

markshannon Oct 28, 2021

Choose a reason for hiding this comment

pablogsal commented Oct 28, 2021 • edited Loading

pablogsal commented Oct 28, 2021

markshannon commented Oct 28, 2021

pablogsal commented Oct 28, 2021 • edited Loading

pablogsal commented Oct 28, 2021

pablogsal commented Oct 27, 2021 •

edited by bedevere-bot

Loading

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021 •

edited

Loading

pablogsal commented Oct 28, 2021 •

edited

Loading