Skip to content

Commit f5c06ea

Browse files
github-actions[bot]taegyunkim
authored andcommitted
fix(profiling): avoid object allocations and frees during memalloc stack unwinding (#16661)
## Description The memory profiler's allocator hook previously used CPython public frame APIs (`PyThreadState_GetFrame`, `PyFrame_GetBack`, `PyFrame_GetCode`) to walk the stack. These APIs return new references, requiring `Py_INCREF`/`Py_DECREF` calls that can trigger reentrant allocations or frees from within the allocator hook — leading to undefined behavior. This PR replaces those APIs with direct struct field access (`_memalloc_frame.h`), using only borrowed references with zero refcount overhead. Also adds a compile-time reentry detection flag (`MEMALLOC_ASSERT_ON_REENTRY`) for test and debug builds. ## Testing - Existing profiling test suites pass - Compile-time assert (`MEMALLOC_ASSERT_ON_REENTRY`) enables early detection in CI/debug builds - Regression test in #16668 reproduces the reentrant crash — tests fail without this fix ## Risks Low — the new frame-walking code reads the same internal structs that CPython's own public APIs wrap, but skips the refcount bookkeeping. The GIL is held during the allocator hook, so the struct reads are safe. ## Additional Notes - `_memalloc_frame.h` has version-specific paths for Python 3.9 through 3.14+ - Non-ASCII function/file names fall back to `<non-ascii>` to avoid `PyUnicode_AsUTF8AndSize` which can allocate a UTF-8 cache - Would refactor ddtrace/profiling/collector/_memalloc_frame.h so that we can share some of these between memory and stack profilers Co-authored-by: taegyun.kim <taegyun.kim@datadoghq.com> (cherry picked from commit 0952bf8) Co-authored-by: Taegyun Kim <taegyun.kim@datadoghq.com>
1 parent cc7fb66 commit f5c06ea

17 files changed

Lines changed: 498 additions & 64 deletions

.gitlab/templates/build-base-venvs.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ build_base_venvs:
1111
DD_PROFILING_NATIVE_TESTS: '1'
1212
DD_USE_SCCACHE: '1'
1313
DD_FAST_BUILD: '1'
14+
DD_PROFILING_MEMALLOC_ASSERT_ON_REENTRY: '1'
1415
rules:
1516
- if: '$CI_COMMIT_REF_NAME == "main"'
1617
variables:

ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,11 @@ class Sample
155155
int64_t line // for ddog_prof_Location
156156
);
157157

158+
// Explicitly mark that one or more frames were dropped without attempting to push them.
159+
// This is useful for callers that perform their own frame-limit checks and want to
160+
// record dropped frames without going through push_frame().
161+
void incr_dropped_frames(size_t count = 1);
162+
158163
// Push an entire PyFrameObject chain to the sample.
159164
// This walks the frame chain and pushes each frame in leaf-to-root order.
160165
// Ownership: this function does not take ownership of the initial `frame`

ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,16 @@ Datadog::Sample::push_frame(function_id function_id, uint64_t address, int64_t l
279279
}
280280
}
281281

282+
// Increments the dropped-frame counter. During export_sample(), if dropped_frames > 0,
283+
// a single synthetic "<N frame(s) omitted>" location is appended to the sample.
284+
// The indicator is added at most once, even if export_sample() is called multiple times
285+
// (guarded by has_dropped_frames_indicator).
286+
void
287+
Datadog::Sample::incr_dropped_frames(size_t count)
288+
{
289+
dropped_frames += count;
290+
}
291+
282292
bool
283293
Datadog::Sample::push_label(const ExportLabelKey key, std::string_view val)
284294
{

ddtrace/profiling/collector/CMakeLists.txt

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,13 @@ if(DEFINED NATIVE_EXTENSION_LOCATION)
129129
endif()
130130
endif()
131131

132+
# Enable allocator-hook reentry assertions in test builds. setup.py turns this on when
133+
# DD_PROFILING_MEMALLOC_ASSERT_ON_REENTRY is set in the build environment.
134+
if(MEMALLOC_ASSERT_ON_REENTRY)
135+
message(STATUS "MEMALLOC_ASSERT_ON_REENTRY enabled: will abort on reentrant allocator hook calls")
136+
target_compile_definitions(${FULL_EXTENSION_NAME} PRIVATE MEMALLOC_ASSERT_ON_REENTRY)
137+
endif()
138+
132139
# Add NDEBUG flag for release builds
133140
if(CMAKE_BUILD_TYPE STREQUAL "Release"
134141
OR CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo"

ddtrace/profiling/collector/_memalloc.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,17 @@ memalloc_free(void* ctx, void* ptr)
4242
if (ptr == NULL)
4343
return;
4444

45-
memalloc_heap_untrack_no_cpython(ptr);
45+
#ifdef MEMALLOC_ASSERT_ON_REENTRY
46+
/* Abort in test builds if we're re-entering from the malloc hook.
47+
* In production we can't abort or skip untrack (skipping would leak
48+
* heap tracker entries), so we just let it proceed — direct struct
49+
* access frame walking avoids calling CPython APIs that could free and is thus safe. */
50+
if (_MEMALLOC_ON_THREAD) {
51+
_memalloc_abort_free_reentry();
52+
}
53+
#endif // MEMALLOC_ASSERT_ON_REENTRY
4654

55+
memalloc_heap_untrack_no_cpython(ptr);
4756
alloc->free(alloc->ctx, ptr);
4857
}
4958

Lines changed: 298 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,298 @@
1+
#pragma once
2+
3+
/* Version-specific frame-walking helpers for the memalloc profiler.
4+
*
5+
* All helpers use direct struct field reads: no new Python references are
6+
* created, no Py_INCREF/Py_DECREF, and no calls that can allocate or free
7+
* Python objects.
8+
* This is critical because these helpers run inside CPython's
9+
* PYMEM_DOMAIN_OBJ allocator hook.
10+
*
11+
* This header must be the first Python header included by a translation unit.
12+
* It defines Py_BUILD_CORE before including Python.h so the CPython internal
13+
* headers below are declared consistently.
14+
*/
15+
16+
#ifdef Py_PYTHON_H
17+
#error "_memalloc_frame.h must be included before Python.h so Py_BUILD_CORE applies to CPython internals"
18+
#endif // Py_PYTHON_H
19+
20+
#define Py_BUILD_CORE
21+
#define PY_SSIZE_T_CLEAN
22+
#include <Python.h>
23+
#include <frameobject.h>
24+
25+
#include "_pymacro.h"
26+
27+
#ifdef Py_GIL_DISABLED
28+
#error "_memalloc frame walking relies on the GIL-held allocator hook and is not yet supported on free-threaded CPython"
29+
#endif // Py_GIL_DISABLED
30+
31+
// AIDEV-TODO: Revisit direct frame walking and heap-tracker synchronization if memalloc adds Py_GIL_DISABLED support.
32+
33+
/* Include CPython internal frame headers for zero-refcount frame walking.
34+
* Python 3.11+: _PyInterpreterFrame is needed for direct frame chain walking.
35+
* Python 3.14+: definition moved to pycore_interpframe_structs.h
36+
* Python 3.11-3.13: definition is in pycore_frame.h */
37+
#ifdef _PY311_AND_LATER
38+
#ifdef _PY314_AND_LATER
39+
#include <internal/pycore_interpframe_structs.h>
40+
#else
41+
#include <internal/pycore_frame.h>
42+
#endif // _PY314_AND_LATER
43+
#include <internal/pycore_code.h>
44+
using memalloc_frame_t = _PyInterpreterFrame;
45+
#else
46+
using memalloc_frame_t = PyFrameObject;
47+
#endif // _PY311_AND_LATER
48+
49+
#ifdef _PY314_AND_LATER
50+
// Expected on our supported 64-bit builds; assert the exact alignment
51+
// invariant that the _PyStackRef tag masking relies on.
52+
static_assert(alignof(PyObject) >= 8,
53+
"PyObject must remain at least 8-byte aligned for _PyStackRef tag masking to be safe");
54+
#endif // _PY314_AND_LATER
55+
56+
/* Return the innermost interpreter frame from the thread state without
57+
* incrementing any reference count. Returns a borrowed frame pointer. */
58+
static inline memalloc_frame_t*
59+
memalloc_get_frame_from_thread_state(PyThreadState* tstate)
60+
{
61+
#ifdef _PY313_AND_LATER
62+
/* Python 3.13+: current_frame is directly on PyThreadState. */
63+
return tstate->current_frame;
64+
#elif defined(_PY311_AND_LATER)
65+
/* Python 3.11-3.12: current_frame is on the _PyCFrame.
66+
* cframe can be NULL while a thread is still being initialized or torn down. */
67+
return tstate->cframe ? tstate->cframe->current_frame : NULL;
68+
#else
69+
/* Pre-3.11: tstate->frame is a public PyFrameObject*. */
70+
return tstate->frame;
71+
#endif // _PY313_AND_LATER
72+
}
73+
74+
/* Return the caller's frame (one level up the call stack) without creating
75+
* a new reference. */
76+
static inline memalloc_frame_t*
77+
memalloc_get_previous_frame(memalloc_frame_t* frame)
78+
{
79+
#ifdef _PY311_AND_LATER
80+
return frame->previous;
81+
#else
82+
return frame->f_back;
83+
#endif // _PY311_AND_LATER
84+
}
85+
86+
/* Return the code object for the frame as a borrowed reference (no INCREF).
87+
* For Python 3.14+, f_executable carries tagged pointer bits that must be
88+
* masked off before treating it as a PyObject*. */
89+
static inline PyCodeObject*
90+
memalloc_get_code_from_frame(memalloc_frame_t* frame)
91+
{
92+
#ifdef _PY314_AND_LATER
93+
/* Python 3.14+: f_executable is a _PyStackRef (tagged pointer).
94+
* Clear the tag bits to recover the PyObject* pointer.
95+
* Masking with ~7 (clearing 3 lowest bits) safely covers all configs
96+
* (debug, free-threading, release), since PyObject* is always aligned
97+
* to at least 8 bytes. */
98+
return (PyCodeObject*)((uintptr_t)frame->f_executable.bits & ~(uintptr_t)7);
99+
#elif defined(_PY313_AND_LATER)
100+
/* Python 3.13: f_executable is an untagged PyObject*. */
101+
return (PyCodeObject*)frame->f_executable;
102+
#elif defined(_PY311_AND_LATER)
103+
/* Python 3.11-3.12: f_code is a direct PyCodeObject*. */
104+
return frame->f_code;
105+
#else
106+
/* Pre-3.11: f_code is a public PyCodeObject*. */
107+
return frame->f_code;
108+
#endif // _PY314_AND_LATER
109+
}
110+
111+
/* Return true for frames that should be skipped during stack walking:
112+
* - frames whose code slot is NULL or not a real PyCodeObject
113+
* - Python 3.12+ "cstack" shim frames used during generator/coroutine entry
114+
* - Python 3.14+ interpreter-owned shim frames
115+
* - Python 3.11 incomplete frames (prev_instr < firsttraceable) */
116+
static inline bool
117+
memalloc_should_skip_frame(memalloc_frame_t* frame)
118+
{
119+
PyObject* code = (PyObject*)memalloc_get_code_from_frame(frame);
120+
if (code == NULL || !PyCode_Check(code)) {
121+
return true;
122+
}
123+
124+
#ifdef _PY312_AND_LATER
125+
return frame->owner != FRAME_OWNED_BY_THREAD && frame->owner != FRAME_OWNED_BY_GENERATOR;
126+
#elif defined(_PY311_AND_LATER)
127+
return _PyFrame_IsIncomplete(frame);
128+
#else
129+
return false;
130+
#endif // _PY312_AND_LATER
131+
}
132+
133+
/* Varint helpers for parsing the 3.11+ location table (PEP 657).
134+
* These read from the co_linetable byte array — pure byte reads,
135+
* no allocations or frees. */
136+
#ifdef _PY311_AND_LATER
137+
static inline int
138+
memalloc_read_varint(const unsigned char* table, Py_ssize_t len, Py_ssize_t* i)
139+
{
140+
Py_ssize_t guard = len - 1;
141+
if (*i >= guard)
142+
return 0;
143+
int val = table[++*i] & 63;
144+
int shift = 0;
145+
while (*i < guard && table[*i] & 64) {
146+
shift += 6;
147+
val |= (table[++*i] & 63) << shift;
148+
}
149+
return val;
150+
}
151+
152+
static inline int
153+
memalloc_read_signed_varint(const unsigned char* table, Py_ssize_t len, Py_ssize_t* i)
154+
{
155+
int val = memalloc_read_varint(table, len, i);
156+
return (val & 1) ? -(val >> 1) : (val >> 1);
157+
}
158+
#endif // _PY311_AND_LATER
159+
160+
/* Return the current line number for the frame by parsing the line table
161+
* directly, without calling PyCode_Addr2Line().
162+
*
163+
* We avoid PyCode_Addr2Line because CPython does not guarantee it is
164+
* allocation-free, and we are called from inside the allocator hook where
165+
* any allocation or free would cause reentrant undefined behaviour.
166+
*
167+
* Instead we parse co_linetable (3.10+) or co_lnotab (3.9) inline.
168+
* The only CPython APIs used are PyBytes_AS_STRING / PyBytes_GET_SIZE,
169+
* which are macros expanding to struct field reads on PyBytesObject
170+
* (ob_sval / ob_size) — guaranteed not to allocate.
171+
*
172+
* The parsing logic is ported from the stack profiler's
173+
* Frame::infer_location() (ddtrace/internal/datadog/profiling/stack/
174+
* src/echion/frame.cc) which handles all supported CPython versions.
175+
*
176+
* AIDEV-TODO: Unify this version-specific line table parsing with the stack
177+
* profiler's Frame::infer_location() implementation so both profilers share a
178+
* single source of truth for CPython location decoding.
179+
*
180+
* Allocation safety: this function only performs pointer arithmetic and byte
181+
* reads from already-owned objects. It does not allocate, decref, or touch
182+
* Python exception state. */
183+
static inline int
184+
memalloc_get_lineno(memalloc_frame_t* frame, PyCodeObject* code)
185+
{
186+
int lasti;
187+
188+
#ifdef _PY313_AND_LATER
189+
/* Python 3.13+: instr_ptr points to the NEXT instruction.
190+
* Result is in _Py_CODEUNIT units. */
191+
lasti = (int)(frame->instr_ptr - 1 - _PyCode_CODE(code));
192+
#elif defined(_PY311_AND_LATER)
193+
/* Python 3.11-3.12: prev_instr points to the last executed instruction.
194+
* Result is in _Py_CODEUNIT units. */
195+
lasti = (int)(frame->prev_instr - _PyCode_CODE(code));
196+
#else
197+
/* Pre-3.11: f_lasti is a byte offset (3.9) or codeunit index (3.10). */
198+
lasti = frame->f_lasti;
199+
#endif // _PY313_AND_LATER
200+
201+
if (lasti < 0) {
202+
return code->co_firstlineno;
203+
}
204+
205+
unsigned int lineno = code->co_firstlineno;
206+
207+
#ifdef _PY311_AND_LATER
208+
/* Python 3.11+: PEP 657 location table in co_linetable.
209+
* Each entry byte: bits[2:0] = (codeunit_delta - 1), bits[6:3] = info code.
210+
* lasti is in _Py_CODEUNIT units, matching the table's bc counter. */
211+
const unsigned char* table = (const unsigned char*)PyBytes_AS_STRING(code->co_linetable);
212+
Py_ssize_t len = PyBytes_GET_SIZE(code->co_linetable);
213+
214+
for (Py_ssize_t i = 0, bc = 0; i < len; i++) {
215+
bc += (table[i] & 7) + 1;
216+
int info_code = (table[i] >> 3) & 15;
217+
switch (info_code) {
218+
case 15: /* No operation */
219+
break;
220+
case 14: /* Long form: signed varint line delta + 3 varints */
221+
lineno += memalloc_read_signed_varint(table, len, &i);
222+
memalloc_read_varint(table, len, &i); /* end_line */
223+
memalloc_read_varint(table, len, &i); /* column */
224+
memalloc_read_varint(table, len, &i); /* end_column */
225+
break;
226+
case 13: /* No column data: signed varint line delta */
227+
lineno += memalloc_read_signed_varint(table, len, &i);
228+
break;
229+
case 12:
230+
case 11:
231+
case 10: /* New lineno: delta = info_code - 10, skip 2 column bytes */
232+
lineno += info_code - 10;
233+
if (i < len - 2)
234+
i += 2;
235+
break;
236+
default: /* Same line, skip 1 column byte */
237+
if (i < len - 1)
238+
i += 1;
239+
break;
240+
}
241+
if (bc > lasti)
242+
break;
243+
}
244+
245+
#elif defined(_PY310_AND_LATER)
246+
/* Python 3.10: PEP 626 line table in co_linetable.
247+
* Pairs of (sdelta, ldelta) bytes. f_lasti is in codeunit units;
248+
* the table bytecode deltas are in byte units, so convert. */
249+
const unsigned char* table = (const unsigned char*)PyBytes_AS_STRING(code->co_linetable);
250+
Py_ssize_t len = PyBytes_GET_SIZE(code->co_linetable);
251+
252+
lasti *= (int)sizeof(_Py_CODEUNIT); /* codeunit index → byte offset */
253+
for (Py_ssize_t i = 0, bc = 0; i < len; i++) {
254+
int sdelta = table[i++];
255+
if (sdelta == 0xff)
256+
break;
257+
bc += sdelta;
258+
int ldelta = table[i];
259+
if (ldelta == 0x80)
260+
ldelta = 0;
261+
else if (ldelta > 0x80)
262+
lineno -= 0x100;
263+
lineno += ldelta;
264+
if (bc > lasti)
265+
break;
266+
}
267+
268+
#else
269+
/* Python 3.9: co_lnotab format — pairs of (bytecode_delta, line_delta)
270+
* unsigned bytes. f_lasti is a byte offset. */
271+
const unsigned char* table = (const unsigned char*)PyBytes_AS_STRING(code->co_lnotab);
272+
Py_ssize_t len = PyBytes_GET_SIZE(code->co_lnotab);
273+
274+
for (Py_ssize_t i = 0, bc = 0; i < len; i++) {
275+
bc += table[i++];
276+
if (bc > lasti)
277+
break;
278+
if (table[i] >= 0x80)
279+
lineno -= 0x100;
280+
lineno += table[i];
281+
}
282+
283+
#endif // _PY311_AND_LATER
284+
285+
return lineno > 0 ? static_cast<int>(lineno) : 0;
286+
}
287+
288+
/* Return the best available function name for a code object.
289+
* co_qualname (Python 3.11+) provides richer context (e.g., Class.method). */
290+
static inline PyObject*
291+
memalloc_get_code_name(PyCodeObject* code)
292+
{
293+
#ifdef _PY311_AND_LATER
294+
return code->co_qualname ? code->co_qualname : code->co_name;
295+
#else
296+
return code->co_name;
297+
#endif // _PY311_AND_LATER
298+
}

0 commit comments

Comments
 (0)