Skip to content

gh-117376: Fix off-by-ones in conversion functions #124301

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Sep 21, 2024

This wasn't caught because the specializations are currently not tested in the free-threaded builds.

To test this, apply the following patch to main:

diff --git a/Python/ceval_macros.h b/Python/ceval_macros.h
index 9e1540674d4..7ca2400529d 100644
--- a/Python/ceval_macros.h
+++ b/Python/ceval_macros.h
@@ -441,28 +441,13 @@ do { \
 /* How much scratch space to give stackref to PyObject* conversion. */
 #define MAX_STACKREF_SCRATCH 10
 
-#ifdef Py_GIL_DISABLED
 #define STACKREFS_TO_PYOBJECTS(ARGS, ARG_COUNT, NAME) \
     /* +1 because vectorcall might use -1 to write self */ \
     PyObject *NAME##_temp[MAX_STACKREF_SCRATCH+1]; \
     PyObject **NAME = _PyObjectArray_FromStackRefArray(ARGS, ARG_COUNT, NAME##_temp + 1);
-#else
-#define STACKREFS_TO_PYOBJECTS(ARGS, ARG_COUNT, NAME) \
-    PyObject **NAME = (PyObject **)ARGS; \
-    assert(NAME != NULL);
-#endif
 
-#ifdef Py_GIL_DISABLED
 #define STACKREFS_TO_PYOBJECTS_CLEANUP(NAME) \
     /* +1 because we +1 previously */ \
-    _PyObjectArray_Free(NAME - 1, NAME##_temp);
-#else
-#define STACKREFS_TO_PYOBJECTS_CLEANUP(NAME) \
-    (void)(NAME);
-#endif
+    // _PyObjectArray_Free(NAME - 1, NAME##_temp);
 
-#ifdef Py_GIL_DISABLED
 #define CONVERSION_FAILED(NAME) ((NAME) == NULL)
-#else
-#define CONVERSION_FAILED(NAME) (0)
-#endif

Note that Python fails to build on main, but passes on this branch.

@rruuaanng
Copy link
Contributor

rruuaanng commented Sep 22, 2024

I think if this patch fails to pass CI on the master branch, it may cause the code base to fail testing after merging.
You should probably try it through CI (Hmm, because I had a performance optimization patch that did that before, and it got shut down).

@Fidget-Spinner
Copy link
Member Author

@rruuaanng the failures are unrelated. If you look at logs it's because it cant fetch LLVM for JIT from the package manager.

@Fidget-Spinner Fidget-Spinner merged commit 198756b into python:main Sep 25, 2024
52 of 59 checks passed
@Fidget-Spinner Fidget-Spinner deleted the fix_some_off_by_ones branch September 25, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants