Skip to content

Commit

Permalink
GH-127705: Add debug mode for _PyStackRefs inspired by HPy debug mo…
Browse files Browse the repository at this point in the history
…de (GH-128121)
  • Loading branch information
markshannon authored Dec 20, 2024
1 parent 78ffba4 commit 128cc47
Show file tree
Hide file tree
Showing 12 changed files with 395 additions and 33 deletions.
6 changes: 6 additions & 0 deletions Include/internal/pycore_interp.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ extern "C" {
#include "pycore_optimizer.h" // _PyOptimizerObject
#include "pycore_obmalloc.h" // struct _obmalloc_state
#include "pycore_qsbr.h" // struct _qsbr_state
#include "pycore_stackref.h" // Py_STACKREF_DEBUG
#include "pycore_tstate.h" // _PyThreadStateImpl
#include "pycore_tuple.h" // struct _Py_tuple_state
#include "pycore_uniqueid.h" // struct _Py_unique_id_pool
Expand Down Expand Up @@ -285,6 +286,11 @@ struct _is {
_PyThreadStateImpl _initial_thread;
// _initial_thread should be the last field of PyInterpreterState.
// See https://github.com/python/cpython/issues/127117.

#if !defined(Py_GIL_DISABLED) && defined(Py_STACKREF_DEBUG)
uint64_t next_stackref;
_Py_hashtable_t *stackref_debug_table;
#endif
};


Expand Down
113 changes: 113 additions & 0 deletions Include/internal/pycore_stackref.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
extern "C" {
#endif

// Define this to get precise tracking of stackrefs.
// #define Py_STACKREF_DEBUG 1

#ifndef Py_BUILD_CORE
# error "this header requires Py_BUILD_CORE define"
#endif
Expand Down Expand Up @@ -49,6 +52,113 @@ extern "C" {
CPython refcounting operations on it!
*/


#if !defined(Py_GIL_DISABLED) && defined(Py_STACKREF_DEBUG)



typedef union _PyStackRef {
uint64_t index;
} _PyStackRef;

#define Py_TAG_BITS 0

PyAPI_FUNC(PyObject *) _Py_stackref_get_object(_PyStackRef ref);
PyAPI_FUNC(PyObject *) _Py_stackref_close(_PyStackRef ref);
PyAPI_FUNC(_PyStackRef) _Py_stackref_create(PyObject *obj, const char *filename, int linenumber);
PyAPI_FUNC(void) _Py_stackref_record_borrow(_PyStackRef ref, const char *filename, int linenumber);
extern void _Py_stackref_associate(PyInterpreterState *interp, PyObject *obj, _PyStackRef ref);

static const _PyStackRef PyStackRef_NULL = { .index = 0 };

#define PyStackRef_None ((_PyStackRef){ .index = 1 } )
#define PyStackRef_False ((_PyStackRef){ .index = 2 })
#define PyStackRef_True ((_PyStackRef){ .index = 3 })

#define LAST_PREDEFINED_STACKREF_INDEX 3

static inline int
PyStackRef_IsNull(_PyStackRef ref)
{
return ref.index == 0;
}

static inline int
PyStackRef_IsTrue(_PyStackRef ref)
{
return _Py_stackref_get_object(ref) == Py_True;
}

static inline int
PyStackRef_IsFalse(_PyStackRef ref)
{
return _Py_stackref_get_object(ref) == Py_False;
}

static inline int
PyStackRef_IsNone(_PyStackRef ref)
{
return _Py_stackref_get_object(ref) == Py_None;
}

static inline PyObject *
_PyStackRef_AsPyObjectBorrow(_PyStackRef ref, const char *filename, int linenumber)
{
_Py_stackref_record_borrow(ref, filename, linenumber);
return _Py_stackref_get_object(ref);
}

#define PyStackRef_AsPyObjectBorrow(REF) _PyStackRef_AsPyObjectBorrow((REF), __FILE__, __LINE__)

static inline PyObject *
PyStackRef_AsPyObjectSteal(_PyStackRef ref)
{
return _Py_stackref_close(ref);
}

static inline _PyStackRef
_PyStackRef_FromPyObjectNew(PyObject *obj, const char *filename, int linenumber)
{
Py_INCREF(obj);
return _Py_stackref_create(obj, filename, linenumber);
}
#define PyStackRef_FromPyObjectNew(obj) _PyStackRef_FromPyObjectNew(_PyObject_CAST(obj), __FILE__, __LINE__)

static inline _PyStackRef
_PyStackRef_FromPyObjectSteal(PyObject *obj, const char *filename, int linenumber)
{
return _Py_stackref_create(obj, filename, linenumber);
}
#define PyStackRef_FromPyObjectSteal(obj) _PyStackRef_FromPyObjectSteal(_PyObject_CAST(obj), __FILE__, __LINE__)

static inline _PyStackRef
_PyStackRef_FromPyObjectImmortal(PyObject *obj, const char *filename, int linenumber)
{
assert(_Py_IsImmortal(obj));
return _Py_stackref_create(obj, filename, linenumber);
}
#define PyStackRef_FromPyObjectImmortal(obj) _PyStackRef_FromPyObjectImmortal(_PyObject_CAST(obj), __FILE__, __LINE__)

static inline void
PyStackRef_CLOSE(_PyStackRef ref)
{
PyObject *obj = _Py_stackref_close(ref);
Py_DECREF(obj);
}

static inline _PyStackRef
_PyStackRef_DUP(_PyStackRef ref, const char *filename, int linenumber)
{
PyObject *obj = _Py_stackref_get_object(ref);
Py_INCREF(obj);
return _Py_stackref_create(obj, filename, linenumber);
}
#define PyStackRef_DUP(REF) _PyStackRef_DUP(REF, __FILE__, __LINE__)

#define PyStackRef_CLOSE_SPECIALIZED(stackref, dealloc) PyStackRef_CLOSE(stackref)

#else

typedef union _PyStackRef {
uintptr_t bits;
} _PyStackRef;
Expand Down Expand Up @@ -200,12 +310,15 @@ static const _PyStackRef PyStackRef_NULL = { .bits = 0 };
#define PyStackRef_IsTrue(ref) (PyStackRef_AsPyObjectBorrow(ref) == Py_True)
#define PyStackRef_IsFalse(ref) (PyStackRef_AsPyObjectBorrow(ref) == Py_False)

#endif

// Converts a PyStackRef back to a PyObject *, converting the
// stackref to a new reference.
#define PyStackRef_AsPyObjectNew(stackref) Py_NewRef(PyStackRef_AsPyObjectBorrow(stackref))

#define PyStackRef_TYPE(stackref) Py_TYPE(PyStackRef_AsPyObjectBorrow(stackref))


#define PyStackRef_CLEAR(op) \
do { \
_PyStackRef *_tmp_op_ptr = &(op); \
Expand Down
1 change: 1 addition & 0 deletions Makefile.pre.in
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ PYTHON_OBJS= \
Python/qsbr.o \
Python/bootstrap_hash.o \
Python/specialize.o \
Python/stackrefs.o \
Python/structmember.o \
Python/symtable.o \
Python/sysmodule.o \
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Adds stackref debugging when ``Py_STACKREF_DEBUG`` is set. Finds all
double-closes and leaks, logging the origin and last borrow.

Inspired by HPy's debug mode. https://docs.hpyproject.org/en/latest/debug-mode.html
4 changes: 2 additions & 2 deletions Objects/frameobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,9 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
if (kind == CO_FAST_FREE) {
// The cell was set when the frame was created from
// the function's closure.
assert(oldvalue.bits != 0 && PyCell_Check(PyStackRef_AsPyObjectBorrow(oldvalue)));
assert(!PyStackRef_IsNull(oldvalue) && PyCell_Check(PyStackRef_AsPyObjectBorrow(oldvalue)));
cell = PyStackRef_AsPyObjectBorrow(oldvalue);
} else if (kind & CO_FAST_CELL && oldvalue.bits != 0) {
} else if (kind & CO_FAST_CELL && !PyStackRef_IsNull(oldvalue)) {
PyObject *as_obj = PyStackRef_AsPyObjectBorrow(oldvalue);
if (PyCell_Check(as_obj)) {
cell = as_obj;
Expand Down
17 changes: 12 additions & 5 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ dummy_func(
assert(Py_REFCNT(left_o) >= 2);
PyStackRef_CLOSE(left);
DEAD(left);
PyObject *temp = PyStackRef_AsPyObjectBorrow(*target_local);
PyObject *temp = PyStackRef_AsPyObjectSteal(*target_local);
PyUnicode_Append(&temp, right_o);
*target_local = PyStackRef_FromPyObjectSteal(temp);
PyStackRef_CLOSE_SPECIALIZED(right, _PyUnicode_ExactDealloc);
Expand Down Expand Up @@ -4509,17 +4509,17 @@ dummy_func(

op(_DO_CALL_FUNCTION_EX, (func_st, unused, callargs_st, kwargs_st if (oparg & 1) -- result)) {
PyObject *func = PyStackRef_AsPyObjectBorrow(func_st);
PyObject *callargs = PyStackRef_AsPyObjectBorrow(callargs_st);
PyObject *kwargs = PyStackRef_AsPyObjectBorrow(kwargs_st);

// DICT_MERGE is called before this opcode if there are kwargs.
// It converts all dict subtypes in kwargs into regular dicts.
assert(kwargs == NULL || PyDict_CheckExact(kwargs));
assert(PyTuple_CheckExact(callargs));
EVAL_CALL_STAT_INC_IF_FUNCTION(EVAL_CALL_FUNCTION_EX, func);
PyObject *result_o;
assert(!_PyErr_Occurred(tstate));
if (opcode == INSTRUMENTED_CALL_FUNCTION_EX) {
PyObject *callargs = PyStackRef_AsPyObjectBorrow(callargs_st);
PyObject *kwargs = PyStackRef_AsPyObjectBorrow(kwargs_st);
assert(kwargs == NULL || PyDict_CheckExact(kwargs));
assert(PyTuple_CheckExact(callargs));
PyObject *arg = PyTuple_GET_SIZE(callargs) > 0 ?
PyTuple_GET_ITEM(callargs, 0) : &_PyInstrumentation_MISSING;
int err = _Py_call_instrumentation_2args(
Expand Down Expand Up @@ -4550,7 +4550,10 @@ dummy_func(
if (Py_TYPE(func) == &PyFunction_Type &&
tstate->interp->eval_frame == NULL &&
((PyFunctionObject *)func)->vectorcall == _PyFunction_Vectorcall) {
PyObject *callargs = PyStackRef_AsPyObjectSteal(callargs_st);
assert(PyTuple_CheckExact(callargs));
PyObject *kwargs = PyStackRef_IsNull(kwargs_st) ? NULL : PyStackRef_AsPyObjectSteal(kwargs_st);
assert(kwargs == NULL || PyDict_CheckExact(kwargs));
Py_ssize_t nargs = PyTuple_GET_SIZE(callargs);
int code_flags = ((PyCodeObject *)PyFunction_GET_CODE(func))->co_flags;
PyObject *locals = code_flags & CO_OPTIMIZED ? NULL : Py_NewRef(PyFunction_GET_GLOBALS(func));
Expand All @@ -4568,6 +4571,10 @@ dummy_func(
frame->return_offset = 1;
DISPATCH_INLINED(new_frame);
}
PyObject *callargs = PyStackRef_AsPyObjectBorrow(callargs_st);
assert(PyTuple_CheckExact(callargs));
PyObject *kwargs = PyStackRef_AsPyObjectBorrow(kwargs_st);
assert(kwargs == NULL || PyDict_CheckExact(kwargs));
result_o = PyObject_Call(func, callargs, kwargs);
}
PyStackRef_XCLOSE(kwargs_st);
Expand Down
67 changes: 53 additions & 14 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ dump_stack(_PyInterpreterFrame *frame, _PyStackRef *stack_pointer)
PyErr_Clear();
}
// Don't call __repr__(), it might recurse into the interpreter.
printf("<%s at %p>", Py_TYPE(obj)->tp_name, (void *)(ptr->bits));
printf("<%s at %p>", Py_TYPE(obj)->tp_name, PyStackRef_AsPyObjectBorrow(*ptr));
}
printf("]\n");
fflush(stdout);
Expand Down Expand Up @@ -805,7 +805,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int



#ifdef Py_DEBUG
#if defined(Py_DEBUG) && !defined(Py_STACKREF_DEBUG)
/* Set these to invalid but identifiable values for debugging. */
entry_frame.f_funcobj = (_PyStackRef){.bits = 0xaaa0};
entry_frame.f_locals = (PyObject*)0xaaa1;
Expand Down Expand Up @@ -1810,27 +1810,48 @@ _PyEvalFramePushAndInit_Ex(PyThreadState *tstate, _PyStackRef func,
{
bool has_dict = (kwargs != NULL && PyDict_GET_SIZE(kwargs) > 0);
PyObject *kwnames = NULL;
PyObject *const *newargs;
_PyStackRef *newargs;
PyObject *const *object_array = NULL;
_PyStackRef stack_array[8];
if (has_dict) {
newargs = _PyStack_UnpackDict(tstate, _PyTuple_ITEMS(callargs), nargs, kwargs, &kwnames);
if (newargs == NULL) {
object_array = _PyStack_UnpackDict(tstate, _PyTuple_ITEMS(callargs), nargs, kwargs, &kwnames);
if (object_array == NULL) {
PyStackRef_CLOSE(func);
goto error;
}
size_t total_args = nargs + PyDict_GET_SIZE(kwargs);
assert(sizeof(PyObject *) == sizeof(_PyStackRef));
newargs = (_PyStackRef *)object_array;
for (size_t i = 0; i < total_args; i++) {
newargs[i] = PyStackRef_FromPyObjectSteal(object_array[i]);
}
}
else {
newargs = &PyTuple_GET_ITEM(callargs, 0);
/* We need to incref all our args since the new frame steals the references. */
for (Py_ssize_t i = 0; i < nargs; ++i) {
Py_INCREF(PyTuple_GET_ITEM(callargs, i));
if (nargs <= 8) {
newargs = stack_array;
}
else {
newargs = PyMem_Malloc(sizeof(_PyStackRef) *nargs);
if (newargs == NULL) {
PyErr_NoMemory();
PyStackRef_CLOSE(func);
goto error;
}
}
/* We need to create a new reference for all our args since the new frame steals them. */
for (Py_ssize_t i = 0; i < nargs; i++) {
newargs[i] = PyStackRef_FromPyObjectNew(PyTuple_GET_ITEM(callargs, i));
}
}
_PyInterpreterFrame *new_frame = _PyEvalFramePushAndInit(
tstate, func, locals,
(_PyStackRef const *)newargs, nargs, kwnames, previous
newargs, nargs, kwnames, previous
);
if (has_dict) {
_PyStack_UnpackDict_FreeNoDecRef(newargs, kwnames);
_PyStack_UnpackDict_FreeNoDecRef(object_array, kwnames);
}
else if (nargs > 8) {
PyMem_Free((void *)newargs);
}
/* No need to decref func here because the reference has been stolen by
_PyEvalFramePushAndInit.
Expand All @@ -1850,21 +1871,39 @@ _PyEval_Vector(PyThreadState *tstate, PyFunctionObject *func,
PyObject* const* args, size_t argcount,
PyObject *kwnames)
{
size_t total_args = argcount;
if (kwnames) {
total_args += PyTuple_GET_SIZE(kwnames);
}
_PyStackRef stack_array[8];
_PyStackRef *arguments;
if (total_args <= 8) {
arguments = stack_array;
}
else {
arguments = PyMem_Malloc(sizeof(_PyStackRef) * total_args);
if (arguments == NULL) {
return PyErr_NoMemory();
}
}
/* _PyEvalFramePushAndInit consumes the references
* to func, locals and all its arguments */
Py_XINCREF(locals);
for (size_t i = 0; i < argcount; i++) {
Py_INCREF(args[i]);
arguments[i] = PyStackRef_FromPyObjectNew(args[i]);
}
if (kwnames) {
Py_ssize_t kwcount = PyTuple_GET_SIZE(kwnames);
for (Py_ssize_t i = 0; i < kwcount; i++) {
Py_INCREF(args[i+argcount]);
arguments[i+argcount] = PyStackRef_FromPyObjectNew(args[i+argcount]);
}
}
_PyInterpreterFrame *frame = _PyEvalFramePushAndInit(
tstate, PyStackRef_FromPyObjectNew(func), locals,
(_PyStackRef const *)args, argcount, kwnames, NULL);
arguments, argcount, kwnames, NULL);
if (total_args > 8) {
PyMem_Free(arguments);
}
if (frame == NULL) {
return NULL;
}
Expand Down
6 changes: 3 additions & 3 deletions Python/ceval_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ do { \
/* How much scratch space to give stackref to PyObject* conversion. */
#define MAX_STACKREF_SCRATCH 10

#ifdef Py_GIL_DISABLED
#if defined(Py_GIL_DISABLED) || defined(Py_STACKREF_DEBUG)
#define STACKREFS_TO_PYOBJECTS(ARGS, ARG_COUNT, NAME) \
/* +1 because vectorcall might use -1 to write self */ \
PyObject *NAME##_temp[MAX_STACKREF_SCRATCH+1]; \
Expand All @@ -461,7 +461,7 @@ do { \
assert(NAME != NULL);
#endif

#ifdef Py_GIL_DISABLED
#if defined(Py_GIL_DISABLED) || defined(Py_STACKREF_DEBUG)
#define STACKREFS_TO_PYOBJECTS_CLEANUP(NAME) \
/* +1 because we +1 previously */ \
_PyObjectArray_Free(NAME - 1, NAME##_temp);
Expand All @@ -470,7 +470,7 @@ do { \
(void)(NAME);
#endif

#ifdef Py_GIL_DISABLED
#if defined(Py_GIL_DISABLED) || defined(Py_STACKREF_DEBUG)
#define CONVERSION_FAILED(NAME) ((NAME) == NULL)
#else
#define CONVERSION_FAILED(NAME) (0)
Expand Down
2 changes: 1 addition & 1 deletion Python/executor_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 128cc47

Please sign in to comment.