[GR-57290][GR-61735][GR-62336] Collected February cleanups.

PullRequest: graalpython/3717

Author: timfel

graalpython/com.oracle.graal.python.cext/src/capi.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * The Universal Permissive License (UPL), Version 1.0
@@ -951,7 +951,7 @@ Py_LOCAL_SYMBOL TruffleContext* TRUFFLE_CONTEXT;
  * that do not work during context shutdown! (This means that it would be safe
  * to share this global across multiple contexts.)
  */
-Py_LOCAL_SYMBOL int32_t graalpy_finalizing;
+Py_LOCAL_SYMBOL int8_t *_graalpy_finalizing = NULL;
 
 PyAPI_FUNC(void) initialize_graal_capi(TruffleEnv* env, void **builtin_closures, GCState *gc) {
     clock_t t = clock();
@@ -1007,10 +1007,16 @@ PyAPI_FUNC(void) initialize_graal_capi(TruffleEnv* env, void **builtin_closures,
 
 /*
  * This function is called from Java during C API initialization to get the
- * pointer `graalpy_finalizing`.
+ * pointer `_graalpy_finalizing`.
  */
-PyAPI_FUNC(int32_t *) GraalPy_get_finalize_capi_pointer() {
-    return &graalpy_finalizing;
+PyAPI_FUNC(int8_t *) GraalPy_get_finalize_capi_pointer() {
+    assert(!_graalpy_finalizing);
+    // We actually leak this memory on purpose. On the Java side, this is
+    // written to in a VM shutdown hook. Once such a hook is registered it
+    // sticks around, so we're leaking those hooks anyway. 1 byte more to leak
+    // does not strike me (timfel) as significant.
+    _graalpy_finalizing = (int8_t *)calloc(1, sizeof(int8_t));
+    return _graalpy_finalizing;
 }
 
 #if ((__linux__ && __GNU_LIBRARY__) || __APPLE__)

graalpython/com.oracle.graal.python.cext/src/capi.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * The Universal Permissive License (UPL), Version 1.0
@@ -157,7 +157,8 @@ PyAPI_DATA(uint32_t) Py_Truffle_Options;
 
 extern THREAD_LOCAL Py_LOCAL_SYMBOL PyThreadState *tstate_current;
 
-extern Py_LOCAL_SYMBOL int graalpy_finalizing;
+extern Py_LOCAL_SYMBOL int8_t *_graalpy_finalizing;
+#define graalpy_finalizing (_graalpy_finalizing != NULL && *_graalpy_finalizing)
 
 /* Flags definitions representing global (debug) options. */
 static MUST_INLINE int PyTruffle_Trace_Memory() {

graalpython/com.oracle.graal.python.cext/src/gcmodule.c

@@ -49,6 +49,18 @@ call_traverse(traverseproc traverse, PyObject *op, visitproc visit, void *arg)
                       Py_TYPE((op))->tp_name);
         return 0;
     } else {
+        if (_PyObject_IsFreed(op)) {
+            PyTruffle_Log(PY_TRUFFLE_LOG_INFO,
+                          "we tried to call tp_traverse on a freed object at %p (ctx %p)!",
+                          op, arg);
+            return 0;
+        }
+        if (_PyObject_IsFreed((PyObject *)Py_TYPE(op))) {
+            PyTruffle_Log(PY_TRUFFLE_LOG_INFO,
+                          "we tried to call tp_traverse on an object at %p with a freed type at %p (ctx %p)!",
+                          op, Py_TYPE(op), arg);
+            return 0;
+        }
         return traverse(op, (visitproc)visit, arg);
     }
 

graalpython/com.oracle.graal.python.cext/src/longobject.c

@@ -1904,98 +1904,99 @@ long_from_binary_base(const char **str, int base, PyLongObject **res)
 PyObject *
 PyLong_FromString(const char *str, char **pend, int base)
 {
-    // GraalPy change: different implementation
-    int negative = 0, error_if_nonzero = 0;
+    int sign = 1, error_if_nonzero = 0;
+    const char *start, *orig_str = str;
+    PyObject *z = NULL;
+    PyObject *strobj;
+    Py_ssize_t slen;
+
     if ((base != 0 && base < 2) || base > 36) {
         PyErr_SetString(PyExc_ValueError,
                         "int() arg 2 must be >= 2 and <= 36");
         return NULL;
     }
-    while (*str != '\0' && Py_ISSPACE(Py_CHARMASK(*str))) {
+    while (*str != '\0' && Py_ISSPACE(*str)) {
         str++;
     }
-    const char *orig_str = str;
     if (*str == '+') {
         ++str;
-    } else if (*str == '-') {
+    }
+    else if (*str == '-') {
         ++str;
-        negative = 1;
+        sign = -1;
     }
     if (base == 0) {
         if (str[0] != '0') {
             base = 10;
-        } else if (str[1] == 'x' || str[1] == 'X') {
+        }
+        else if (str[1] == 'x' || str[1] == 'X') {
             base = 16;
-            str += 2;
-        } else if (str[1] == 'o' || str[1] == 'O') {
+        }
+        else if (str[1] == 'o' || str[1] == 'O') {
             base = 8;
-            str += 2;
-        } else if (str[1] == 'b' || str[1] == 'B') {
+        }
+        else if (str[1] == 'b' || str[1] == 'B') {
             base = 2;
-            str += 2;
-        } else {
+        }
+        else {
             /* "old" (C-style) octal literal, now invalid.
                it might still be zero though */
             error_if_nonzero = 1;
             base = 10;
         }
     }
-
-    char* numberStart = str;
-    int overflow = 0;
-    int digits = 0;
-    char prev;
-    long value;
-    while (1) {
+    if (str[0] == '0' &&
+        ((base == 16 && (str[1] == 'x' || str[1] == 'X')) ||
+         (base == 8  && (str[1] == 'o' || str[1] == 'O')) ||
+         (base == 2  && (str[1] == 'b' || str[1] == 'B')))) {
+        str += 2;
+        /* One underscore allowed here. */
         if (*str == '_') {
-            if (prev == '_') {
-                goto error;
-            }
-        } else {
-            unsigned char digit = _PyLong_DigitValue[Py_CHARMASK(*str)];
-            if (digit >= base) {
+            ++str;
+        }
+    }
+
+    // GraalPy change: remove the CPython code below and upcall quickly. Only
+    // optimization we do is to check if this may be a small-ish integer. There
+    // is a small integer cache and if we're lucky we can just return from
+    // that.
+    // In base 2, up to 8 digits may be a small integer, in base 36 8 digits
+    // still fit easily in 64 bits
+    for (int i = 0; i < 8; i++) {
+        char c = str[i];
+        if (c == '\0') {
+            int errsv = errno;
+            char *endptr;
+            long long result = strtoll(str, &endptr, base);
+            if (error_if_nonzero && result != 0) {
+                // let upcall handle the error reporting
+                base = 0;
                 break;
             }
-            long new_value = value * base - digit;
-            if (new_value > value) {
-                // overflow
-                overflow = 1;
+            // POSIX.1-2008: strtoll must not set errno on success, and set
+            // *endptr to str when no conversion is performed
+            if (errno == 0 && str != endptr) {
+                while (*endptr && Py_ISSPACE(*endptr)) {
+                    endptr++;
+                }
+                if (*endptr == '\0') {
+                    z = PyLong_FromLongLong(sign < 0 ? -result : result);
+                }
             }
-            value = new_value;
+            errno = errsv;
+            break;
+        } else if (!(isascii(c) && isalnum(c))) {
+            // cannot be a base 2 to 36 digit
+            break;
         }
-        prev = *str;
-        ++str;
-        ++digits;
-    }
-
-    if (prev == '_') {
-        /* Trailing underscore not allowed. */
-        goto error;
-    }
-    while (*str != '\0' && Py_ISSPACE(Py_CHARMASK(*str))) {
-        str++;
     }
-    if (pend != NULL) {
-        *pend = str;
-    }
-    if (value == LONG_MIN && !negative) {
-        overflow = 1;
-    }
-
-    if (overflow || (error_if_nonzero && value != 0)) {
-        if (error_if_nonzero) {
-            base = 0;
+    if (!z) {
+        z = GraalPyTruffleLong_FromString(orig_str, base);
+        if (z) {
+            // TODO: we should probably set the **pend out argument
         }
-        return GraalPyTruffleLong_FromString(orig_str, base);
-    } else {
-        return PyLong_FromLong(negative ? value : -value);
     }
-
- error:
-    PyErr_Format(PyExc_ValueError,
-                 "invalid literal for int() with base %d: %.200R",
-                 base, PyUnicode_FromString(str));
-    return NULL;
+    return z;
 }
 
 #if 0 // GraalPy change

graalpython/com.oracle.graal.python.cext/src/moduleobject.c

@@ -1,4 +1,4 @@
-/* Copyright (c) 2022, 2024, Oracle and/or its affiliates.
+/* Copyright (c) 2022, 2025, Oracle and/or its affiliates.
  * Copyright (C) 1996-2022 Python Software Foundation
  *
  * Licensed under the PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
@@ -258,7 +258,6 @@ _PyModule_CreateInitialized(PyModuleDef* module, int module_api_version)
     return (PyObject*)m;
 }
 
-#if 0 // GraalPy change
 PyObject *
 PyModule_FromDefAndSpec2(PyModuleDef* def, PyObject *spec, int module_api_version)
 {
@@ -340,8 +339,12 @@ PyModule_FromDefAndSpec2(PyModuleDef* def, PyObject *spec, int module_api_versio
     }
 
     if (PyModule_Check(m)) {
-        ((PyModuleObject*)m)->md_state = NULL;
-        ((PyModuleObject*)m)->md_def = def;
+        // GraalPy change: avoid direct struct access
+        GraalPy_set_PyModuleObject_md_state((PyModuleObject *)m, NULL);
+        GraalPy_set_PyModuleObject_md_def((PyModuleObject *)m, def);
+        // End of GraalPy change, CPython code was next two lines
+        // ((PyModuleObject*)m)->md_state = NULL;
+        // ((PyModuleObject*)m)->md_def = def;
     } else {
         if (def->m_size > 0 || def->m_traverse || def->m_clear || def->m_free) {
             PyErr_Format(
@@ -361,10 +364,16 @@ PyModule_FromDefAndSpec2(PyModuleDef* def, PyObject *spec, int module_api_versio
     }
 
     if (def->m_methods != NULL) {
-        ret = _add_methods_to_object(m, nameobj, def->m_methods);
-        if (ret != 0) {
-            goto error;
+        // GraalPy change: use PyModule_AddFunctions instead of _add_methods_to_object
+        if (PyModule_AddFunctions(m, def->m_methods) != 0) {
+            Py_DECREF(m);
+            return NULL;
         }
+        // End of GraalPy change, original code below
+        // ret = _add_methods_to_object(m, nameobj, def->m_methods);
+        // if (ret != 0) {
+        //     goto error;
+        // }
     }
 
     if (def->m_doc != NULL) {
@@ -397,16 +406,27 @@ PyModule_ExecDef(PyObject *module, PyModuleDef *def)
 
     if (def->m_size >= 0) {
         PyModuleObject *md = (PyModuleObject*)module;
-        if (md->md_state == NULL) {
-            /* Always set a state pointer; this serves as a marker to skip
-             * multiple initialization (importlib.reload() is no-op) */
-            md->md_state = PyMem_Malloc(def->m_size);
-            if (!md->md_state) {
+        // GraalPy change: avoid direct struct access
+        if (GraalPy_get_PyModuleObject_md_state(md) == NULL) {
+            void* md_state = PyMem_Malloc(def->m_size);
+            if (!md_state) {
                 PyErr_NoMemory();
                 return -1;
             }
-            memset(md->md_state, 0, def->m_size);
+            memset(md_state, 0, def->m_size);
+            GraalPy_set_PyModuleObject_md_state(md, md_state);
         }
+        // End GraalPy change, original code below
+        // if (md->md_state == NULL) {
+        //     /* Always set a state pointer; this serves as a marker to skip
+        //      * multiple initialization (importlib.reload() is no-op) */
+        //     md->md_state = PyMem_Malloc(def->m_size);
+        //     if (!md->md_state) {
+        //         PyErr_NoMemory();
+        //         return -1;
+        //     }
+        //     memset(md->md_state, 0, def->m_size);
+        // }
     }
 
     if (def->m_slots == NULL) {
@@ -447,7 +467,6 @@ PyModule_ExecDef(PyObject *module, PyModuleDef *def)
     }
     return 0;
 }
-#endif // GraalPy change
 
 int
 PyModule_AddFunctions(PyObject *m, PyMethodDef *functions)

graalpython/com.oracle.graal.python.cext/src/object.c

@@ -372,7 +372,6 @@ PyObject_Print(PyObject *op, FILE *fp, int flags)
     return ret;
 }
 
-#if 0 // GraalPy change
 /* For debugging convenience.  Set a breakpoint here and call it from your DLL */
 void
 _Py_BreakPoint(void)
@@ -389,9 +388,14 @@ _Py_BreakPoint(void)
 int
 _PyObject_IsFreed(PyObject *op)
 {
+    if (points_to_py_handle_space(op)) {
+        return Graal_PyTruffleObject_IsFreed(op);
+    }
+#if 0 // GraalPy change
     if (_PyMem_IsPtrFreed(op) || _PyMem_IsPtrFreed(Py_TYPE(op))) {
         return 1;
     }
+#endif
     /* ignore op->ob_ref: its value can have be modified
        by Py_INCREF() and Py_DECREF(). */
 #ifdef Py_TRACE_REFS
@@ -405,11 +409,14 @@ _PyObject_IsFreed(PyObject *op)
     return 0;
 }
 
-
 /* For debugging convenience.  See Misc/gdbinit for some useful gdb hooks */
 void
 _PyObject_Dump(PyObject* op)
 {
+    if (points_to_py_handle_space(op)) {
+        Graal_PyTruffleObject_Dump(op);
+        return;
+    }
     if (_PyObject_IsFreed(op)) {
         /* It seems like the object memory has been freed:
            don't access it to prevent a segmentation fault. */
@@ -448,6 +455,7 @@ _PyObject_Dump(PyObject* op)
     fflush(stderr);
 }
 
+#if 0 // GraalPy change
 PyObject *
 PyObject_Repr(PyObject *v)
 {

graalpython/com.oracle.graal.python.test/src/org/graalvm/python/embedding/cext/test/MultiContextCExtTest.java

@@ -243,15 +243,9 @@ public void testCreatingVenvForMulticontext() throws IOException {
             } catch (PolyglotException e) {
                 assertTrue("We rely on sys.prefix", e.getMessage().contains("sys.prefix must be a str"));
             }
-            // Fifth one does not work because we don't have the venv configured
-            try {
-                c5.eval(code);
-                fail("should not reach here");
-            } catch (PolyglotException e) {
-                assertTrue("We need a venv", e.getMessage().contains("sys.prefix must point to a venv"));
-            }
-            // Using a context without isolation in the same process needs to use LLVM
-            assertFalse("have not had a context use LLVM, yet", log.truffleLog.toString().contains("as bitcode"));
+            // Fifth works even without a venv
+            c5.eval(code);
+            // Using a context without isolation in the same process does not work
             try {
                 c0.eval(code);
                 fail("should not reach here");