Skip to content

Commit 38416fe

Browse files
hetaozdhpablogsal
andauthored
gh-158539: Fix exception mode missing handlers in generators/coroutines (#158581)
* gh-158539: Fix exception mode missing handlers in generators/coroutines The sampling profiler's exception mode decided whether a thread was handling an exception by reading the embedded PyThreadState.exc_state. Generators, coroutines and async generators repoint tstate->exc_info at their own _PyErr_StackItem while they run, so an except block running in one of them (or in a function they call) stored the exception in that item instead, and was never sampled. Follow tstate->exc_info and its previous_item chain, mirroring _PyErr_GetTopmostException(), and export the two debug offsets needed to walk the chain from remote memory. The common case where exc_info points at the embedded exc_state keeps the existing zero-extra-read fast path. * gh-158539: Handle deep exception chains without changing debug offsets * gh-158539: Use portable static assertion messages * gh-158539: Keep layout assertions with debug-offset validation * gh-158539: Use the platform guard for in-process inspection tests --------- Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
1 parent 802f145 commit 38416fe

4 files changed

Lines changed: 299 additions & 11 deletions

File tree

‎Lib/test/test_external_inspection.py‎

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3293,6 +3293,242 @@ def test_finally_no_exception_no_flag(self):
32933293
self._check_exception_status(p, thread_tid, expect_exception=False)
32943294

32953295

3296+
@skip_if_not_supported
3297+
class TestExceptionDetectionInProcess(RemoteInspectionTestBase):
3298+
"""gh-158539: HAS_EXCEPTION for handlers running in generators/coroutines.
3299+
3300+
``TestExceptionDetectionScenarios`` samples a child process and therefore
3301+
needs subprocess debugging permissions. These tests inspect the current
3302+
process with ``RemoteUnwinder`` and only need self-inspection, so they also
3303+
run on macOS without special entitlements.
3304+
"""
3305+
3306+
@classmethod
3307+
def setUpClass(cls):
3308+
try:
3309+
RemoteUnwinder(os.getpid(), all_threads=True).get_stack_trace()
3310+
except PermissionError as exc:
3311+
raise unittest.SkipTest(f"self-inspection is unavailable: {exc}")
3312+
3313+
def _check_running_handler(
3314+
self, target, expect_exception, *, mode=PROFILING_MODE_ALL,
3315+
skip_non_matching_threads=False,
3316+
):
3317+
"""Run *target* in a thread and check its HAS_EXCEPTION flag.
3318+
3319+
*target* receives ``(ready, stop)`` events and must signal ``ready``
3320+
only once it is executing inside the code region under test, then keep
3321+
running until ``stop`` is set.
3322+
"""
3323+
stop = threading.Event()
3324+
ready = threading.Event()
3325+
failure = []
3326+
3327+
def runner():
3328+
try:
3329+
target(ready, stop)
3330+
except BaseException as exc:
3331+
failure.append(exc)
3332+
ready.set()
3333+
3334+
thread = threading.Thread(target=runner, daemon=True)
3335+
thread.start()
3336+
try:
3337+
self.assertTrue(ready.wait(SHORT_TIMEOUT), "handler never started")
3338+
self.assertFalse(failure, f"handler raised {failure!r}")
3339+
3340+
unwinder = RemoteUnwinder(
3341+
os.getpid(),
3342+
all_threads=True,
3343+
mode=mode,
3344+
skip_non_matching_threads=skip_non_matching_threads,
3345+
)
3346+
observed = []
3347+
for _ in busy_retry(SHORT_TIMEOUT):
3348+
with contextlib.suppress(*TRANSIENT_ERRORS):
3349+
statuses = self._get_thread_statuses(unwinder.get_stack_trace())
3350+
status = statuses.get(thread.native_id)
3351+
if status is None:
3352+
continue
3353+
has_exception = bool(status & THREAD_STATUS_HAS_EXCEPTION)
3354+
observed.append(has_exception)
3355+
if has_exception == expect_exception:
3356+
break
3357+
self.assertTrue(
3358+
observed, "target thread status was never observed"
3359+
)
3360+
self.assertIn(
3361+
expect_exception,
3362+
observed,
3363+
f"HAS_EXCEPTION was never {expect_exception} while the "
3364+
f"handler was running (observed {observed})",
3365+
)
3366+
finally:
3367+
stop.set()
3368+
thread.join(SHORT_TIMEOUT)
3369+
3370+
def _busy_until_stopped(self, ready, stop):
3371+
ready.set()
3372+
while not stop.is_set():
3373+
time.sleep(0.001)
3374+
3375+
def test_handler_in_function(self):
3376+
def target(ready, stop):
3377+
try:
3378+
raise ValueError("test")
3379+
except ValueError:
3380+
self._busy_until_stopped(ready, stop)
3381+
3382+
self._check_running_handler(target, expect_exception=True)
3383+
3384+
def test_handler_in_generator(self):
3385+
def target(ready, stop):
3386+
def gen():
3387+
try:
3388+
raise ValueError("test")
3389+
except ValueError:
3390+
self._busy_until_stopped(ready, stop)
3391+
yield
3392+
3393+
for _ in gen():
3394+
pass
3395+
3396+
self._check_running_handler(target, expect_exception=True)
3397+
3398+
def test_handler_in_genexpr_callee(self):
3399+
def target(ready, stop):
3400+
def callee():
3401+
try:
3402+
raise ValueError("test")
3403+
except ValueError:
3404+
self._busy_until_stopped(ready, stop)
3405+
3406+
list(callee() for _ in range(1))
3407+
3408+
self._check_running_handler(target, expect_exception=True)
3409+
3410+
def test_handler_in_coroutine(self):
3411+
async def coro(ready, stop):
3412+
try:
3413+
raise ValueError("test")
3414+
except ValueError:
3415+
self._busy_until_stopped(ready, stop)
3416+
3417+
def target(ready, stop):
3418+
asyncio.run(coro(ready, stop))
3419+
3420+
self._check_running_handler(target, expect_exception=True)
3421+
3422+
def test_handler_in_callee_from_coroutine(self):
3423+
def callee(ready, stop):
3424+
try:
3425+
raise ValueError("test")
3426+
except ValueError:
3427+
self._busy_until_stopped(ready, stop)
3428+
3429+
async def coro(ready, stop):
3430+
callee(ready, stop)
3431+
3432+
def target(ready, stop):
3433+
asyncio.run(coro(ready, stop))
3434+
3435+
self._check_running_handler(target, expect_exception=True)
3436+
3437+
def test_outer_handler_while_generator_runs(self):
3438+
"""A generator with no handler of its own must not hide the outer one.
3439+
3440+
``exc_info`` points at the generator's empty ``_PyErr_StackItem`` whose
3441+
``previous_item`` is the thread's ``exc_state``, so the profiler has to
3442+
walk the chain to find the exception ``sys.exception()`` reports.
3443+
"""
3444+
def target(ready, stop):
3445+
def gen():
3446+
self._busy_until_stopped(ready, stop)
3447+
yield
3448+
3449+
try:
3450+
raise ValueError("outer")
3451+
except ValueError:
3452+
for _ in gen():
3453+
pass
3454+
3455+
self._check_running_handler(target, expect_exception=True)
3456+
3457+
def test_generator_without_exception(self):
3458+
def target(ready, stop):
3459+
def gen():
3460+
self._busy_until_stopped(ready, stop)
3461+
yield
3462+
3463+
for _ in gen():
3464+
pass
3465+
3466+
self._check_running_handler(target, expect_exception=False)
3467+
3468+
def test_outer_handler_while_nested_generators_run(self):
3469+
def target(ready, stop):
3470+
def gen(depth):
3471+
if depth:
3472+
yield from gen(depth - 1)
3473+
else:
3474+
self._busy_until_stopped(ready, stop)
3475+
yield
3476+
3477+
try:
3478+
raise ValueError("outer")
3479+
except ValueError:
3480+
for _ in gen(32):
3481+
pass
3482+
3483+
self._check_running_handler(
3484+
target,
3485+
expect_exception=True,
3486+
mode=PROFILING_MODE_EXCEPTION,
3487+
skip_non_matching_threads=True,
3488+
)
3489+
3490+
def test_generator_finally_after_except(self):
3491+
"""The handled exception is cleared before the generator's finally."""
3492+
def target(ready, stop):
3493+
def gen():
3494+
try:
3495+
raise ValueError("test")
3496+
except ValueError:
3497+
pass
3498+
finally:
3499+
self._busy_until_stopped(ready, stop)
3500+
yield
3501+
3502+
for _ in gen():
3503+
pass
3504+
3505+
self._check_running_handler(target, expect_exception=False)
3506+
3507+
def test_exception_mode_filter_keeps_generator_handler(self):
3508+
"""The exception-mode thread filter must not drop a generator handler.
3509+
3510+
This mirrors what ``--mode=exception`` actually does: threads without
3511+
HAS_EXCEPTION are skipped before their stack is unwound.
3512+
"""
3513+
def target(ready, stop):
3514+
def gen():
3515+
try:
3516+
raise ValueError("test")
3517+
except ValueError:
3518+
self._busy_until_stopped(ready, stop)
3519+
yield
3520+
3521+
for _ in gen():
3522+
pass
3523+
3524+
self._check_running_handler(
3525+
target,
3526+
expect_exception=True,
3527+
mode=PROFILING_MODE_EXCEPTION,
3528+
skip_non_matching_threads=True,
3529+
)
3530+
3531+
32963532
@requires_remote_subprocess_debugging()
32973533
class TestFrameCaching(RemoteInspectionTestBase):
32983534
"""Test that frame caching produces correct results.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :mod:`profiling.sampling` exception mode discarding samples for code
2+
running inside an ``except`` block in a generator or coroutine. The remote
3+
debugger now follows ``tstate->exc_info`` and its ``previous_item`` chain
4+
instead of only reading the embedded ``exc_state``, matching the exception
5+
that :func:`sys.exception` reports.

‎Modules/_remote_debugging/debug_offsets_validation.h‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,15 @@ static_assert(
4848
PY_REMOTE_ASYNC_DEBUG_OFFSETS_TOTAL_SIZE,
4949
"Update _remote_debugging validation for _Py_AsyncioModuleDebugOffsets");
5050

51+
/* Derive unexported offsets from adjacent fields to keep the debug-offset
52+
* table compatible across patch releases. */
53+
static_assert(offsetof(PyThreadState, exc_info) ==
54+
offsetof(PyThreadState, current_exception) + sizeof(uintptr_t),
55+
"exc_info must immediately follow current_exception");
56+
static_assert(offsetof(_PyErr_StackItem, previous_item) ==
57+
offsetof(_PyErr_StackItem, exc_value) + sizeof(uintptr_t),
58+
"previous_item must immediately follow exc_value");
59+
5160
/*
5261
* This logic lives in a private header because it is shared by module.c and
5362
* asyncio.c. Keep the helpers static inline so they stay local to those users
@@ -249,14 +258,15 @@ validate_fixed_field(
249258
#define PY_REMOTE_DEBUG_RUNTIME_STATE_FIELDS(APPLY, buffer_size) \
250259
APPLY(runtime_state, interpreters_head, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size)
251260

261+
/* current_exception also covers the adjacent exc_info pointer. */
252262
#define PY_REMOTE_DEBUG_THREAD_STATE_FIELDS(APPLY, buffer_size) \
253263
APPLY(thread_state, native_thread_id, sizeof(unsigned long), _Alignof(long), buffer_size); \
254264
APPLY(thread_state, interp, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
255265
APPLY(thread_state, datastack_chunk, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
256266
APPLY(thread_state, status, FIELD_SIZE(PyThreadState, _status), _Alignof(unsigned int), buffer_size); \
257267
APPLY(thread_state, holds_gil, sizeof(int), _Alignof(int), buffer_size); \
258268
APPLY(thread_state, gil_requested, sizeof(int), _Alignof(int), buffer_size); \
259-
APPLY(thread_state, current_exception, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
269+
APPLY(thread_state, current_exception, 2 * sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
260270
APPLY(thread_state, thread_id, sizeof(unsigned long), _Alignof(long), buffer_size); \
261271
APPLY(thread_state, next, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
262272
APPLY(thread_state, current_frame, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
@@ -351,10 +361,11 @@ _PyRemoteDebug_ValidateDebugOffsetsLayout(struct _Py_DebugOffsets *debug_offsets
351361
PY_REMOTE_DEBUG_THREAD_STATE_FIELDS(
352362
PY_REMOTE_DEBUG_VALIDATE_FIELD,
353363
SIZEOF_THREAD_STATE);
364+
/* exc_value also covers the adjacent previous_item pointer. */
354365
PY_REMOTE_DEBUG_VALIDATE_FIXED_FIELD(
355366
err_stackitem,
356367
exc_value,
357-
sizeof(uintptr_t),
368+
2 * sizeof(uintptr_t),
358369
_Alignof(uintptr_t),
359370
sizeof(_PyErr_StackItem));
360371
PY_REMOTE_DEBUG_VALIDATE_NESTED_FIELD(

‎Modules/_remote_debugging/threads.c‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
#include <sys/wait.h>
1818
#endif
1919

20+
/* Bound traversal of corrupted remote exception chains. */
21+
#define MAX_EXCEPTION_CHAIN_DEPTH (2 << 15)
22+
2023
/* ============================================================================
2124
* THREAD ITERATION FUNCTIONS
2225
* ============================================================================ */
@@ -436,16 +439,49 @@ unwind_stack_for_thread(
436439
has_exception = 1;
437440
}
438441

439-
// Check exc_state.exc_value (exception being handled in except block)
440-
// exc_state is embedded in PyThreadState, so we read it directly from
441-
// the thread state buffer. This catches most cases; nested exception
442-
// handlers where exc_info points elsewhere are rare.
442+
// Generators and coroutines use their own exception stack items.
443+
// Follow exc_info to find the innermost handler, as sys.exception() does.
443444
if (!has_exception) {
444-
uintptr_t exc_value = GET_MEMBER(uintptr_t, ts,
445-
unwinder->debug_offsets.thread_state.exc_state +
446-
unwinder->debug_offsets.err_stackitem.exc_value);
447-
if (exc_value != 0) {
448-
has_exception = 1;
445+
uintptr_t exc_info = GET_MEMBER(uintptr_t, ts,
446+
unwinder->debug_offsets.thread_state.current_exception +
447+
sizeof(uintptr_t));
448+
uintptr_t exc_state_addr =
449+
*current_tstate + unwinder->debug_offsets.thread_state.exc_state;
450+
uintptr_t exc_value_offset =
451+
unwinder->debug_offsets.err_stackitem.exc_value;
452+
uintptr_t previous_item_offset =
453+
exc_value_offset + sizeof(uintptr_t);
454+
455+
for (int depth = 0; exc_info != 0 && depth < MAX_EXCEPTION_CHAIN_DEPTH;
456+
depth++)
457+
{
458+
if (exc_info == exc_state_addr) {
459+
// Bottom of the chain: the stack item embedded in the thread
460+
// state, which is already in the local thread state buffer.
461+
uintptr_t exc_value = GET_MEMBER(uintptr_t, ts,
462+
unwinder->debug_offsets.thread_state.exc_state +
463+
exc_value_offset);
464+
if (exc_value != 0) {
465+
has_exception = 1;
466+
}
467+
break;
468+
}
469+
uintptr_t exc_value = 0;
470+
if (read_ptr(unwinder, exc_info + exc_value_offset, &exc_value) < 0) {
471+
PyErr_Clear(); // Best effort: treat as no active exception
472+
break;
473+
}
474+
if (exc_value != 0) {
475+
has_exception = 1;
476+
break;
477+
}
478+
uintptr_t previous_item = 0;
479+
if (read_ptr(unwinder, exc_info + previous_item_offset,
480+
&previous_item) < 0) {
481+
PyErr_Clear();
482+
break;
483+
}
484+
exc_info = previous_item;
449485
}
450486
}
451487

0 commit comments

Comments
 (0)