Do not re-initialize interpreter during finalization. #150137
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kinds of issues including stolen GIL and segfaults.
|
@llvm/pr-subscribers-mlir Author: Anton Korobeynikov (asl) ChangesThis causes all kinds of issues including stolen GIL and segfaults. Full diff: https://github.com/llvm/llvm-project/pull/150137.diff 1 Files Affected:
diff --git a/mlir/lib/Bindings/Python/IRAttributes.cpp b/mlir/lib/Bindings/Python/IRAttributes.cpp
index 8f79caf08a6d0..a783b0346385e 100644
--- a/mlir/lib/Bindings/Python/IRAttributes.cpp
+++ b/mlir/lib/Bindings/Python/IRAttributes.cpp
@@ -1428,6 +1428,12 @@ class PyDenseIntElementsAttribute
}
};
+// Check if the python version is less than 3.13. Py_IsFinalizing is a part
+// of stable ABI since 3.13 and before it was available as _Py_IsFinalizing.
+#if PY_VERSION_HEX < 0x030d0000
+#define Py_IsFinalizing _Py_IsFinalizing
+#endif
+
class PyDenseResourceElementsAttribute
: public PyConcreteAttribute<PyDenseResourceElementsAttribute> {
public:
@@ -1474,7 +1480,7 @@ class PyDenseResourceElementsAttribute
// The userData is a Py_buffer* that the deleter owns.
auto deleter = [](void *userData, const void *data, size_t size,
size_t align) {
- if (!Py_IsInitialized())
+ if (!Py_IsFinalizing() && !Py_IsInitialized())
Py_Initialize();
Py_buffer *ownedView = static_cast<Py_buffer *>(userData);
nb::gil_scoped_acquire gil;
|
asl
requested review from
stellaraccident,
weiweichen,
naibaf7 and
laszlokindrat
|
See #124832 (comment) for explanation I believe this fixes llvm/torch-mlir#4157 and iree-org/iree#20102 plus other hangs and segfaults being workarounded in other projects (see e.g. last comment in tenstorrent/tt-torch@0decda6) |
|
This is the same PR as #149414. |
makslevental
added a commit
that referenced
this pull request
Jul 25, 2025
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]>
|
Fixed by #150561 |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 25, 2025
…zer (#150561) This PR melds llvm/llvm-project#150137 and llvm/llvm-project#149414 *and* partially reverts llvm/llvm-project#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" llvm/llvm-project#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]>
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
…50561) This PR melds llvm#150137 and llvm#149414 *and* partially reverts llvm#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" llvm#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]>
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jul 28, 2025
…50561) This PR melds llvm#150137 and llvm#149414 *and* partially reverts llvm#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" llvm#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]>
Indeed. Have not noticed that it was already there, search returned nothing :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This causes all kinds of issues including stolen GIL and segfaults.