[GR-71615] Enable -O3 and LTO for C API.

fangerer · fangerer · commit c90ddaae7aef · 2025-12-09T10:24:43.000Z
PullRequest: graalpython/4144
diff --git a/graalpython/com.oracle.graal.python.cext/CMakeLists.txt b/graalpython/com.oracle.graal.python.cext/CMakeLists.txt
@@ -55,6 +55,10 @@ set(TARGET_LIBPYTHON "python-native")
 # common variables and compile/link options (for all build targets)
 ######################################################################
 
+set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE TRUE)
+set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELWITHDEBINFO TRUE)
+set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_DEBUG FALSE)
+
 if (MSVC)
   SET(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "" FORCE)
   SET(CFLAGS_WARNINGS /Wall /WX
@@ -109,7 +113,6 @@ endif()
 
 # preprocessor defines for all platforms
 add_compile_definitions(
-    NDEBUG
     WITH_FREELISTS=1
 )
 
diff --git a/graalpython/com.oracle.graal.python.cext/include/internal/pycore_object.h b/graalpython/com.oracle.graal.python.cext/include/internal/pycore_object.h
@@ -367,9 +367,12 @@ _PyObject_IS_GC(PyObject *obj)
 static inline size_t
 _PyType_PreHeaderSize(PyTypeObject *tp)
 {
-    // GraalPy change: we put only one pointer for dict, we don't store it inlined [GR-61996]
+    /* Note: the pre-header size must have a 16-byte alignment. This is
+       necessary for higher optimization levels which assume the alignment
+       (inferred from the struct size) and emit instructions that require
+       this alignment. */
     return _PyType_IS_GC(tp) * sizeof(PyGC_Head) +
-        _PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * /* 2 * */ sizeof(PyObject *);
+        _PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * 2 * sizeof(PyObject *);
 }
 
 void _PyObject_GC_Link(PyObject *op);
diff --git a/graalpython/com.oracle.graal.python.cext/src/abstract.c b/graalpython/com.oracle.graal.python.cext/src/abstract.c
@@ -11,8 +11,8 @@
 #if 0 // GraalPy change
 #include "pycore_call.h"          // _PyObject_CallNoArgs()
 #include "pycore_ceval.h"         // _Py_EnterRecursiveCallTstate()
-#include "pycore_object.h"        // _Py_CheckSlotResult()
 #endif // GraalPy change
+#include "pycore_object.h"        // _Py_CheckSlotResult()
 #include "pycore_long.h"          // _Py_IsNegative
 #include "pycore_pyerrors.h"      // _PyErr_Occurred()
 #include "pycore_pystate.h"       // _PyThreadState_GET()
diff --git a/graalpython/com.oracle.graal.python.cext/src/object.c b/graalpython/com.oracle.graal.python.cext/src/object.c
@@ -2882,18 +2882,9 @@ void _Py_SetRefcnt(PyObject* obj, Py_ssize_t cnt) {
         return;
     }
     PyObject *dest;
-    if (points_to_py_handle_space(obj))
-    {
+    if (points_to_py_handle_space(obj)) {
         dest = pointer_to_stub(obj);
-#ifndef NDEBUG
-        if (GraalPyPrivate_Debug_CAPI())
-        {
-            set_PyObject_ob_refcnt(obj, cnt);
-        }
-#endif
-    }
-    else
-    {
+    } else {
         dest = obj;
     }
     dest->ob_refcnt = cnt;
diff --git a/graalpython/com.oracle.graal.python.cext/src/tupleobject.c b/graalpython/com.oracle.graal.python.cext/src/tupleobject.c
@@ -1326,12 +1326,19 @@ PyObject* GraalPyPrivate_Tuple_Alloc(PyTypeObject* type, Py_ssize_t nitems) {
     if (alloc  == NULL) {
         return PyErr_NoMemory();
     }
+
+    // GraalPy change: zero whole object
+    memset(alloc, '\0', size + presize);
+
     obj = (PyObject *)(alloc + presize);
     if (presize) {
-        // GraalPy change: different header layout, no GC link
-        ((PyObject **)alloc)[0] = NULL;
+        // GraalPy change: different header layout
+        // ((PyObject **)alloc)[0] = NULL;
+        // ((PyObject **)alloc)[1] = NULL;
+        _PyObject_GC_Link(obj);
     }
-    memset(obj, '\0', size);
+    // GraalPy change: whole memory is zero'd above
+    // memset(obj, '\0', size);
 
     if (type->tp_itemsize == 0) {
         _PyObject_Init(obj, type);
diff --git a/graalpython/com.oracle.graal.python.cext/src/unicodeobject.c b/graalpython/com.oracle.graal.python.cext/src/unicodeobject.c
@@ -12823,7 +12823,9 @@ _PyUnicodeWriter_WriteASCIIString(_PyUnicodeWriter *writer,
     if (len == -1)
         len = strlen(ascii);
 
+#if 0 // GraalPy change
     assert(ucs1lib_find_max_char((const Py_UCS1*)ascii, (const Py_UCS1*)ascii + len) < 128);
+#endif // GraalPy change
 
     if (writer->buffer == NULL && !writer->overallocate) {
         PyObject *str;
diff --git a/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_gc.py b/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_gc.py
@@ -192,6 +192,7 @@ def test_native_class(self):
 ID_OBJ12 = 10
 ID_OBJ13 = 11
 ID_OBJ14 = 12
+ID_OBJ15 = 13
 
 # don't rely on deterministic Java GC behavior by default on GraalPy
 RELY_ON_GC = os.environ.get("RELY_ON_GC", not GRAALPY)
@@ -204,13 +205,15 @@ def test_native_class(self):
 
 if GRAALPY:
     get_handle_table_id = __graalpython__.get_handle_table_id
+    storage_to_native = __graalpython__.storage_to_native
     def assert_is_strong(x): assert __graalpython__.is_strong_handle_table_ref(x)
     def assert_is_weak(x): assert not __graalpython__.is_strong_handle_table_ref(x)
 else:
     # just that the test is compatible with CPython
-    def get_handle_table_id(object): return -1
-    def assert_is_strong(x): pass
-    def assert_is_weak(x): pass
+    def get_handle_table_id(_): return -1
+    def storage_to_native(_): pass
+    def assert_is_strong(_): pass
+    def assert_is_weak(_): pass
 
 class TestGCRefCycles(unittest.TestCase):
     def _trigger_gc(self):
@@ -357,6 +360,7 @@ def assert_is_freed(id): pass
         obj10 = TestCycle0(ID_OBJ10)
         obj11 = TestCycle0(ID_OBJ11)
         obj14 = TestCycle0(ID_OBJ14)
+        obj15 = TestCycle0(ID_OBJ15)
 
         # Legend
         # '=>'
@@ -462,6 +466,19 @@ def assert_is_freed(id): pass
         htid_l4 = get_handle_table_id(l4)
         del l4
 
+        # establish cycle:   l5 => obj15 =ht=> l5
+        # update_refs:       11     1
+        # subtract_refs:     10     0
+        # move_unreachable:  10     11
+        # update_refs:       11     11
+        # subtract_refs:     10     10
+        # commit_weak_cand:  l5 => obj15 =ht-> l5
+        l5 = [obj15]
+        storage_to_native(l5)
+        obj15.set_obj(l5)
+        htid_l5 = get_handle_table_id(l5)
+        del obj15
+
         # everything should still be alive
         assert_is_alive(ID_OBJ2)
         assert_is_alive(ID_OBJ3)
@@ -474,16 +491,19 @@ def assert_is_freed(id): pass
         assert_is_alive(ID_OBJ10)
         assert_is_alive(ID_OBJ11)
         assert_is_alive(ID_OBJ14)
+        assert_is_alive(ID_OBJ15)
         assert_is_strong(htid_l)
         assert_is_strong(htid_l1)
         assert_is_strong(htid_l2)
         assert_is_strong(htid_l3)
         assert_is_strong(htid_l4)
+        assert_is_strong(htid_l5)
         assert_is_strong(htid_d0)
 
         del obj2, l, obj3
         del obj4, obj5
         del obj6, d0, obj7
+        del l5
 
         ####################################### GC #######################################
         self._trigger_gc()
@@ -511,6 +531,7 @@ def assert_is_freed(id): pass
         assert_is_strong(htid_l1)
         assert_is_strong(htid_l4)
         assert_is_strong(htid_d0)
+        assert_is_freed(ID_OBJ15)
 
         rescued_obj4 = l1[0]
         del l1
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java
@@ -1555,7 +1555,7 @@ static Object doNative(Object weakCandidates,
                         GC_LOGGER.fine(PythonUtils.formatJString("Transitioning to weak reference to break a reference cycle for %s, refcount=%d",
                                         abstractObjectNativeWrapper.ref, abstractObjectNativeWrapper.getRefCount()));
                     }
-                    updateRefNode.clearStrongRef(inliningTarget, abstractObjectNativeWrapper);
+                    updateRefNode.clearStrongRefButKeepInGCList(inliningTarget, abstractObjectNativeWrapper);
                 }
 
                 // next = GC_NEXT(gc)
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiGCSupport.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiGCSupport.java
@@ -44,6 +44,7 @@
 
 import java.util.logging.Level;
 
+import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.GCListRemoveNodeGen;
 import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.PyObjectGCDelNodeGen;
 import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.PExternalFunctionWrapper;
@@ -55,6 +56,7 @@
 import com.oracle.graal.python.builtins.objects.cext.structs.CStructAccess.FreeNode;
 import com.oracle.graal.python.builtins.objects.cext.structs.CStructs;
 import com.oracle.graal.python.runtime.PythonContext;
+import com.oracle.graal.python.runtime.PythonOptions;
 import com.oracle.graal.python.util.PythonUtils;
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.GenerateCached;
@@ -165,12 +167,24 @@ public static long executeUncached(long opUntagged) {
             return GCListRemoveNodeGen.getUncached().execute(null, opUntagged);
         }
 
+        /**
+         * Remove the Python object denoted by the given {@code PyObject*} pointer from the GC list
+         * it is currently contained in. This will apply {@code AS_GC(op)} first.
+         */
+        public final void executeOp(Node inliningTarget, long op) {
+            if (PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
+                execute(inliningTarget, HandlePointerConverter.pointerToStub(op));
+            }
+        }
+
         public abstract long execute(Node inliningTarget, long opUntagged);
 
         @Specialization
         static long doGeneric(long opUntagged,
                         @Cached(inline = false) CStructAccess.ReadI64Node readI64Node,
                         @Cached(inline = false) CStructAccess.WriteLongNode writeLongNode) {
+            assert PythonLanguage.get(null).getEngineOption(PythonOptions.PythonGC);
+
             // issue a log message before doing the first memory access
             if (GC_LOGGER.isLoggable(Level.FINER)) {
                 GC_LOGGER.finer(PythonUtils.formatJString("attempting to remove 0x%x from GC generation", opUntagged));
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java
@@ -2152,30 +2152,48 @@ static Object doIt(Object object) {
     public abstract static class UpdateStrongRefNode extends Node {
 
         public final void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, long refCount) {
-            execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT);
+            execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT, false);
         }
 
-        public final void clearStrongRef(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
-            execute(inliningTarget, wrapper, false);
+        /**
+         * Makes the handle table reference of the given wrapper weak but keeps the native object
+         * stub in the GC list (if currently contained in any). The only valid use case for this
+         * method is when iterating over all objects of a GC list, calling this method on each
+         * object and in the end, dropping the whole GC list.
+         */
+        public final void clearStrongRefButKeepInGCList(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
+            execute(inliningTarget, wrapper, false, true);
         }
 
-        public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong);
+        public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList);
 
         @Specialization
-        static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong,
+        static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList,
                         @Cached InlinedConditionProfile hasRefProfile,
-                        @Cached PyObjectGCTrackNode gcTrackNode) {
+                        @Cached PyObjectGCTrackNode gcTrackNode,
+                        @Cached GCListRemoveNode gcListRemoveNode) {
+            assert CompilerDirectives.isPartialEvaluationConstant(keepInGcList);
+
             PythonObjectReference ref;
             if (hasRefProfile.profile(inliningTarget, (ref = wrapper.ref) != null)) {
                 assert ref.pointer == wrapper.getNativePointer();
                 if (setStrong && !ref.isStrongReference()) {
                     ref.setStrongReference(wrapper);
                     if (ref.gc && PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
                         // gc = AS_GC(op)
-                        long gc = wrapper.getNativePointer() - CStructs.PyGC_Head.size();
+                        long gc = ref.pointer - CStructs.PyGC_Head.size();
                         gcTrackNode.execute(inliningTarget, gc);
                     }
                 } else if (!setStrong && ref.isStrongReference()) {
+                    /*
+                     * As soon as the reference is made weak, we remove it from the GC list because
+                     * there are ways to iterate a GC list (e.g. 'PyUnstable_GC_VisitObjects') and
+                     * while doing so, the objects may be accessed. Since weakly referenced objects
+                     * may die any time, this could lead to dangling pointers being used.
+                     */
+                    if (!keepInGcList && ref.gc) {
+                        gcListRemoveNode.executeOp(inliningTarget, ref.pointer);
+                    }
                     ref.setStrongReference(null);
                 }
             }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlotGetAttr.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlotGetAttr.java
@@ -45,6 +45,8 @@
 import static com.oracle.graal.python.nodes.SpecialMethodNames.T___GETATTR__;
 import static com.oracle.graal.python.runtime.exception.PythonErrorType.AttributeError;
 
+import java.lang.ref.Reference;
+
 import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes.AsCharPointerNode;
 import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.ExternalFunctionInvokeNode;
@@ -263,6 +265,8 @@ static Object callNative(VirtualFrame frame, TpSlots slots, TpSlotNative slot, O
             } finally {
                 if (isGetAttr) {
                     freeNode.free(nameArg);
+                } else {
+                    Reference.reachabilityFence(nameArg);
                 }
             }
             return checkResultNode.execute(threadState, T___GETATTR__, toPythonNode.execute(result));
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlotSetAttr.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlotSetAttr.java
@@ -45,6 +45,8 @@
 import static com.oracle.graal.python.nodes.SpecialMethodNames.T___DELATTR__;
 import static com.oracle.graal.python.nodes.SpecialMethodNames.T___SETATTR__;
 
+import java.lang.ref.Reference;
+
 import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.Python3Core;
 import com.oracle.graal.python.builtins.PythonBuiltinClassType;
@@ -281,6 +283,8 @@ static void callNative(VirtualFrame frame, TpSlots slots, TpSlotNative slot, Obj
             } finally {
                 if (isSetAttr) {
                     freeNode.free(nameArg);
+                } else {
+                    Reference.reachabilityFence(nameArg);
                 }
             }
             checkResultNode.execute(threadState, T___SETATTR__, result);
diff --git a/mx.graalpython/suite.py b/mx.graalpython/suite.py
@@ -670,6 +670,11 @@
                             "libc": ["default"],
                         },
                         "cmakeConfig": {
+                            # Possible/useful build types:
+                            # "Release": highest opt level, no debugging symbols, no assertions, -DNDEBUG, etc.
+                            # "RelWithDebInfo": medium opt level, with debugging symbols, no assertions, -DNDEBUG
+                            # "Debug": no opt level, debugging symbols, assertions, ...
+                            "CMAKE_BUILD_TYPE": "Release",
                             "CAPI_INC_DIR": "<output_root:com.oracle.graal.python>/jni_gen",
                             "PYCONFIG_INCLUDE_DIR": "<output_root:graalpy-pyconfig>/<os>-<arch>/<multitarget_libc_selection>",
                             "TRUFFLE_NFI_H_INC": "<path:com.oracle.truffle.nfi.native>/include",
@@ -701,6 +706,11 @@
                             {"libc": ["musl"], "variant": ["swcfi"]},
                         ],
                         "cmakeConfig": {
+                            # Possible/useful build types:
+                            # "Release": highest opt level (e.g. -O3, -lto), no debugging symbols, no assertions, -DNDEBUG, etc.
+                            # "RelWithDebInfo": medium opt level (e.g. -O2, -lto), with debugging symbols, no assertions, -DNDEBUG
+                            # "Debug": no opt level, debugging symbols, assertions, ...
+                            "CMAKE_BUILD_TYPE": "Release",
                             "CAPI_INC_DIR": "<output_root:com.oracle.graal.python>/jni_gen",
                             "PYCONFIG_INCLUDE_DIR": "<output_root:graalpy-pyconfig>/<os>-<arch>/<multitarget_libc_selection>/",
                             "TRUFFLE_NFI_H_INC": "<path:com.oracle.truffle.nfi.native>/include",

Original file line number	Diff line number	Diff line change
`@@ -367,9 +367,12 @@ _PyObject_IS_GC(PyObject *obj)`
`367`	`367`	`static inline size_t`
`368`	`368`	`_PyType_PreHeaderSize(PyTypeObject *tp)`
`369`	`369`	`{`
`370`		`- // GraalPy change: we put only one pointer for dict, we don't store it inlined [GR-61996]`
	`370`	`+ /* Note: the pre-header size must have a 16-byte alignment. This is`
	`371`	`+ necessary for higher optimization levels which assume the alignment`
	`372`	`+ (inferred from the struct size) and emit instructions that require`
	`373`	`+ this alignment. */`
`371`	`374`	`return _PyType_IS_GC(tp) * sizeof(PyGC_Head) +`
`372`		`- _PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * /* 2 * / sizeof(PyObject );`
	`375`	`+ _PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * 2 * sizeof(PyObject *);`
`373`	`376`	`}`
`374`	`377`
`375`	`378`	`void _PyObject_GC_Link(PyObject *op);`
Original file line number	Diff line number	Diff line change
`@@ -1555,7 +1555,7 @@ static Object doNative(Object weakCandidates,`
`1555`	`1555`	`GC_LOGGER.fine(PythonUtils.formatJString("Transitioning to weak reference to break a reference cycle for %s, refcount=%d",`
`1556`	`1556`	`abstractObjectNativeWrapper.ref, abstractObjectNativeWrapper.getRefCount()));`
`1557`	`1557`	`}`
`1558`		`- updateRefNode.clearStrongRef(inliningTarget, abstractObjectNativeWrapper);`
	`1558`	`+ updateRefNode.clearStrongRefButKeepInGCList(inliningTarget, abstractObjectNativeWrapper);`
`1559`	`1559`	`}`
`1560`	`1560`
`1561`	`1561`	`// next = GC_NEXT(gc)`