Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.6k
gh-116738: Make cmath module thread-safe#142161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
colesbury left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Generally looks good. It's subinterpreter specific and not really free threading related (i.e., you'll have the same data races with the default GIL enabled build).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Core_and_Builtins/2025-12-01-10-03-08.gh-issue-116738.972YsG.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
yoney commentedDec 1, 2025
Thank you for the review, Sam!
Yes, I agree, this is related to subinterpreters. I was mainly checking the module for ft-python, which is why I created the PR undergh-116738 to mark it checked. |
| } | ||
| staticint | ||
| init_special_value_tables(void*Py_UNUSED(arg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, do we really need this? Why not initialize these values at compile time?
E.g.:
diff --git a/Modules/cmathmodule.c b/Modules/cmathmodule.cindex aee3e4f343d..dc409845876 100644--- a/Modules/cmathmodule.c+++ b/Modules/cmathmodule.c@@ -162,8 +162,16 @@ special_type(double d) errno = ERANGE where the overflow floating-point signal should be raised. */--static Py_complex acos_special_values[7][7];+#define D(R,I) (Py_complex){(R), (I)}+static Py_complex acos_special_values[7][7] = {+ D(P34,INF), D(P,INF), D(P,INF), D(P,-INF), D(P,-INF), D(P34,-INF), D(N,INF),+ D(P12,INF), D(U,U), D(U,U), D(U,U), D(U,U), D(P12,-INF), D(N,N),+ D(P12,INF), D(U,U), D(P12,0.), D(P12,-0.), D(U,U), D(P12,-INF), D(P12,N),+ D(P12,INF), D(U,U), D(P12,0.), D(P12,-0.), D(U,U), D(P12,-INF), D(P12,N),+ D(P12,INF), D(U,U), D(U,U), D(U,U), D(U,U), D(P12,-INF), D(N,N),+ D(P14,INF), D(0.,INF), D(0.,INF), D(0.,-INF), D(0.,-INF), D(P14,-INF), D(N,INF),+ D(N,INF), D(N,N), D(N,N), D(N,N), D(N,N), D(N,-INF), D(N,N),+}; /*[clinic input] cmath.acos -> Py_complex_protected@@ -1207,16 +1215,6 @@ cmath_exec(PyObject *mod) #define INIT_SPECIAL_VALUES(NAME, BODY) { Py_complex* p = (Py_complex*)NAME; BODY } #define C(REAL, IMAG) p->real = REAL; p->imag = IMAG; ++p;- INIT_SPECIAL_VALUES(acos_special_values, {- C(P34,INF) C(P,INF) C(P,INF) C(P,-INF) C(P,-INF) C(P34,-INF) C(N,INF)- C(P12,INF) C(U,U) C(U,U) C(U,U) C(U,U) C(P12,-INF) C(N,N)- C(P12,INF) C(U,U) C(P12,0.) C(P12,-0.) C(U,U) C(P12,-INF) C(P12,N)- C(P12,INF) C(U,U) C(P12,0.) C(P12,-0.) C(U,U) C(P12,-INF) C(P12,N)- C(P12,INF) C(U,U) C(U,U) C(U,U) C(U,U) C(P12,-INF) C(N,N)- C(P14,INF) C(0.,INF) C(0.,INF) C(0.,-INF) C(0.,-INF) C(P14,-INF) C(N,INF)- C(N,INF) C(N,N) C(N,N) C(N,N) C(N,N) C(N,-INF) C(N,N)- })- INIT_SPECIAL_VALUES(acosh_special_values, { C(INF,-P34) C(INF,-P) C(INF,-P) C(INF,P) C(INF,P) C(INF,P34) C(INF,N) C(INF,-P12) C(U,U) C(U,U) C(U,U) C(U,U) C(INF,P12) C(N,N)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@skirpichev Good point! We don’t really need this. I mainly wanted to avoid moving the init data around and keep the code changes minimal for an easier review but we can update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
keep the code changes minimal for an easier review
I understand.
we can update it
I think that pr's with code refactoring only - usually rejected by core devs. While this is an opportunity both to simplify the code and fix a real issue.
Uh oh!
There was an error while loading.Please reload this page.
While investigating thread safety in the
cmathmodule, I found that the module is generally thread-safe. However, there is a potential race condition during the initialization of the special value tables incmath_exec(). This is usually safe, but with subinterpreters and free-threading, a race condition could occur.Since the fix is relatively straightforward, I have included it in this PR.
I have also created atest to run under ThreadSanitizer. However, I am not including the test in the current PR because, although this PR addresses the issue in the
cmathmodule, ThreadSanitizer still reports other issues below when subinterpreters and free-threading are used together. I plan to address those issues.cc:@mpage@colesbury