Skip to content

Commit

Permalink
[Bugfix][FFI] Typo fix of IncRef to DecRef (#16021)
Browse files Browse the repository at this point in the history
Propagation of Python exceptions across C++ stack frames was
introduced in #15596.  This commit
primarily fixes a typo in the initial implementation, where
`Py_IncRef` was used instead of `Py_DecRef`.

In addition, this PR resolves errors that were exposed by this typo
fix, which caused test failures in
`tests/python/unittest/test_crt.py::test_compile_runtime`.  These were
due to use of the `Py_IncRef` and `Py_DecRef` functions on threads
that hadn't acquired the GIL.  This usage error has been corrected for
both the ctypes and cython FFI handling.
  • Loading branch information
Lunderberg authored Nov 6, 2023
1 parent 02d4df7 commit ffa0033
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 4 deletions.
2 changes: 2 additions & 0 deletions python/tvm/_ffi/_ctypes/packed_func.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ def _init_pythonapi_inc_def_ref():
register_func = _LIB.TVMBackendRegisterEnvCAPI
register_func(c_str("Py_IncRef"), ctypes.pythonapi.Py_IncRef)
register_func(c_str("Py_DecRef"), ctypes.pythonapi.Py_DecRef)
register_func(c_str("PyGILState_Ensure"), ctypes.pythonapi.PyGILState_Ensure)
register_func(c_str("PyGILState_Release"), ctypes.pythonapi.PyGILState_Release)


_init_pythonapi_inc_def_ref()
4 changes: 3 additions & 1 deletion python/tvm/_ffi/_cython/packed_func.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import ctypes
import traceback
from cpython cimport Py_INCREF, Py_DECREF
from cpython cimport Py_INCREF, Py_DECREF, PyGILState_Ensure, PyGILState_Release
from numbers import Number, Integral
from ..base import string_types, py2cerror
from ..runtime_ctypes import DataType, Device, TVMByteArray, ObjectRValueRef
Expand Down Expand Up @@ -381,5 +381,7 @@ def _init_pythonapi_inc_def_ref():
register_func = TVMBackendRegisterEnvCAPI
register_func(c_str("Py_IncRef"), <void*>_py_incref_wrapper)
register_func(c_str("Py_DecRef"), <void*>_py_decref_wrapper)
register_func(c_str("PyGILState_Ensure"), <void*>PyGILState_Ensure)
register_func(c_str("PyGILState_Release"), <void*>PyGILState_Release)

_init_pythonapi_inc_def_ref()
44 changes: 41 additions & 3 deletions src/runtime/registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,16 @@ class EnvCAPIRegistry {
*/
F_Py_IncDefRef py_dec_ref = nullptr;

/*!
\brief PyGILState_Ensure function
*/
void* (*py_gil_state_ensure)() = nullptr;

/*!
\brief PyGILState_Release function
*/
void (*py_gil_state_release)(void*) = nullptr;

static EnvCAPIRegistry* Global() {
static EnvCAPIRegistry* inst = new EnvCAPIRegistry();
return inst;
Expand All @@ -161,6 +171,10 @@ class EnvCAPIRegistry {
Update(symbol_name, &py_inc_ref, fptr);
} else if (symbol_name == "Py_DecRef") {
Update(symbol_name, &py_dec_ref, fptr);
} else if (symbol_name == "PyGILState_Ensure") {
Update(symbol_name, &py_gil_state_ensure, fptr);
} else if (symbol_name == "PyGILState_Release") {
Update(symbol_name, &py_gil_state_release, fptr);
} else {
LOG(FATAL) << "Unknown env API " << symbol_name;
}
Expand All @@ -177,15 +191,17 @@ class EnvCAPIRegistry {
}

void IncRef(void* python_obj) {
WithGIL context(this);
ICHECK(py_inc_ref) << "Attempted to call Py_IncRef through EnvCAPIRegistry, "
<< "but Py_IncRef wasn't registered";
(*py_inc_ref)(python_obj);
}

void DecRef(void* python_obj) {
ICHECK(py_inc_ref) << "Attempted to call Py_IncRef through EnvCAPIRegistry, "
<< "but Py_IncRef wasn't registered";
(*py_inc_ref)(python_obj);
WithGIL context(this);
ICHECK(py_dec_ref) << "Attempted to call Py_DefRef through EnvCAPIRegistry, "
<< "but Py_DefRef wasn't registered";
(*py_dec_ref)(python_obj);
}

private:
Expand All @@ -198,6 +214,28 @@ class EnvCAPIRegistry {
}
target[0] = ptr_casted;
}

struct WithGIL {
explicit WithGIL(EnvCAPIRegistry* self) : self(self) {
ICHECK(self->py_gil_state_ensure) << "Attempted to acquire GIL through EnvCAPIRegistry, "
<< "but PyGILState_Ensure wasn't registered";
ICHECK(self->py_gil_state_release) << "Attempted to acquire GIL through EnvCAPIRegistry, "
<< "but PyGILState_Release wasn't registered";
gil_state = self->py_gil_state_ensure();
}
~WithGIL() {
if (self && gil_state) {
self->py_gil_state_release(gil_state);
}
}
WithGIL(const WithGIL&) = delete;
WithGIL(WithGIL&&) = delete;
WithGIL& operator=(const WithGIL&) = delete;
WithGIL& operator=(WithGIL&&) = delete;

EnvCAPIRegistry* self = nullptr;
void* gil_state = nullptr;
};
};

void EnvCheckSignals() { EnvCAPIRegistry::Global()->CheckSignals(); }
Expand Down

0 comments on commit ffa0033

Please sign in to comment.