Skip to content

Commit 2177448

Browse files
maksleventalaslmamanain
authored
[mlir][python] fix PyDenseResourceElementsAttribute finalizer (#150561)
This PR melds #150137 and #149414 *and* partially reverts #124832. The summary is the `PyDenseResourceElementsAttribute` finalizer/deleter has/had two problems 1. wasn't threadsafe (can be called from a different thread than that which currently holds the GIL) 2. can be called while the interpreter is "not initialized" #124832 for some reason decides to re-initialize the interpreter to avoid case 2 and runs afoul of the fact that `Py_IsInitialized` can be false during the finalization of the interpreter itself (e.g., at the end of a script). I don't know why this decision was made (I missed the PR) but I believe we should never be calling [Py_Initialize](https://docs.python.org/3/c-api/init.html#c.Py_Initialize): > In an application \*\*\*\***embedding Python**\*\*\*\*, this should be called before using any other Python/C API functions **but we aren't embedding Python**! So therefore we will only be in case 2 when the interpreter is being finalized and in that case we should just leak the buffer. Note, [lldb](https://github.com/llvm/llvm-project/blob/548ca9e97673a168023a616d311d901ca04b29a3/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp#L81-L93) does a similar sort of thing for its finalizers. Co-authored-by: Anton Korobeynikov <[email protected]> Co-authored-by: Max Manainen <[email protected]> Co-authored-by: Anton Korobeynikov <[email protected]> Co-authored-by: Max Manainen <[email protected]>
1 parent 8005c6a commit 2177448

File tree

2 files changed

+30
-5
lines changed

2 files changed

+30
-5
lines changed

mlir/lib/Bindings/Python/IRAttributes.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616
#include "NanobindUtils.h"
1717
#include "mlir-c/BuiltinAttributes.h"
1818
#include "mlir-c/BuiltinTypes.h"
19-
#include "mlir/Bindings/Python/NanobindAdaptors.h"
2019
#include "mlir/Bindings/Python/Nanobind.h"
20+
#include "mlir/Bindings/Python/NanobindAdaptors.h"
2121
#include "llvm/ADT/ScopeExit.h"
2222
#include "llvm/Support/raw_ostream.h"
2323

@@ -1428,6 +1428,12 @@ class PyDenseIntElementsAttribute
14281428
}
14291429
};
14301430

1431+
// Check if the python version is less than 3.13. Py_IsFinalizing is a part
1432+
// of stable ABI since 3.13 and before it was available as _Py_IsFinalizing.
1433+
#if PY_VERSION_HEX < 0x030d0000
1434+
#define Py_IsFinalizing _Py_IsFinalizing
1435+
#endif
1436+
14311437
class PyDenseResourceElementsAttribute
14321438
: public PyConcreteAttribute<PyDenseResourceElementsAttribute> {
14331439
public:
@@ -1474,8 +1480,9 @@ class PyDenseResourceElementsAttribute
14741480
// The userData is a Py_buffer* that the deleter owns.
14751481
auto deleter = [](void *userData, const void *data, size_t size,
14761482
size_t align) {
1477-
if (!Py_IsInitialized())
1478-
Py_Initialize();
1483+
if (Py_IsFinalizing())
1484+
return;
1485+
assert(Py_IsInitialized() && "expected interpreter to be initialized");
14791486
Py_buffer *ownedView = static_cast<Py_buffer *>(userData);
14801487
nb::gil_scoped_acquire gil;
14811488
PyBuffer_Release(ownedView);

mlir/test/python/ir/array_attributes.py

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ def testGetDenseElementsUnsupported():
3131
# CHECK: unimplemented array format conversion from format:
3232
print(e)
3333

34+
3435
# CHECK-LABEL: TEST: testGetDenseElementsUnSupportedTypeOkIfExplicitTypeProvided
3536
@run
3637
def testGetDenseElementsUnSupportedTypeOkIfExplicitTypeProvided():
@@ -41,8 +42,9 @@ def testGetDenseElementsUnSupportedTypeOkIfExplicitTypeProvided():
4142
# realistic example would be a NumPy extension type like the bfloat16
4243
# type from the ml_dtypes package, which isn't a dependency of this
4344
# test.
44-
attr = DenseElementsAttr.get(array.view(np.datetime64),
45-
type=IntegerType.get_signless(64))
45+
attr = DenseElementsAttr.get(
46+
array.view(np.datetime64), type=IntegerType.get_signless(64)
47+
)
4648
# CHECK: dense<{{\[}}[1, 2, 3], [4, 5, 6]]> : tensor<2x3xi64>
4749
print(attr)
4850
# CHECK: {{\[}}[1 2 3]
@@ -135,6 +137,7 @@ def testGetDenseElementsFromListMixedTypes():
135137
# Splats.
136138
################################################################################
137139

140+
138141
# CHECK-LABEL: TEST: testGetDenseElementsSplatInt
139142
@run
140143
def testGetDenseElementsSplatInt():
@@ -617,3 +620,18 @@ def test_attribute(context, mview):
617620
# CHECK: BACKING MEMORY DELETED
618621
# CHECK: EXIT FUNCTION
619622
print("EXIT FUNCTION")
623+
624+
625+
# CHECK-LABEL: TEST: testDanglingResource
626+
print("TEST: testDanglingResource")
627+
# see https://github.com/llvm/llvm-project/pull/149414, https://github.com/llvm/llvm-project/pull/150137, https://github.com/llvm/llvm-project/pull/150561
628+
# This error occurs only when there is an alive context with a DenseResourceElementsAttr
629+
# in the end of the program, so we put it here without an encapsulating function.
630+
ctx = Context()
631+
632+
with ctx, Location.unknown():
633+
DenseResourceElementsAttr.get_from_buffer(
634+
memoryview(np.array([1, 2, 3])),
635+
"some_resource",
636+
RankedTensorType.get((3,), IntegerType.get_signed(32)),
637+
)

0 commit comments

Comments
 (0)