Changes made in sprint for PLDI deadline. by mjp41 · Pull Request #62 · mjp41/cpython

mjp41 · 2025-11-26T10:48:58Z

No description provided.

Co-authored-by: xFrednet <xFrednet@gmail.com>

xFrednet

There are several loose ends, but all of them are pretty well documented in TODO comments.

Most of my comments are just thoughts, small additions and two TODO's for me :)

xFrednet · 2025-11-26T13:07:48Z

Include/internal/pycore_freelist_state.h

+#define MAXFREELIST(x) x
+//#define MAXFREELIST(x) 0


Should this be part of the change set or is this an artifact of development/Debugging?

If this should be kept, it should be used for all _MAXFREELIST values in this file

I found this useful for debugging memory safety issues with Asan. The freelists hide UAF and double-free that Asan would detect.
I think this is something that we could try to push into CPython for the sanitizer builds.

I am happy to drop from this PR. It isn't essential. I just would sooner not lose the code.

xFrednet · 2025-11-26T13:09:19Z

Include/internal/pycore_moduleobject.h

+    // For immutable modules to find the mutable state and
+    // for logging purposes after md_dict is cleared
+    PyObject *md_name;
+    int md_frozen;


FIXME: @xFrednet, Investigate if this field can be avoided with the change of the Module object type as part of the pre-freeze hook.

xFrednet · 2025-11-26T13:36:30Z

Include/refcount.h

 PyAPI_FUNC(void) _Py_IncRef(PyObject *);
 PyAPI_FUNC(void) _Py_DecRef(PyObject *);

+// TODO(Immutable): Should this not be defined in the LIMITED_API?


I believe they need to be defined even with the LIMITED_API due to the dec and inc ref macros/functions being defined in the header. The leading _ should indicate that this function is still private and shouldn't be used directly

xFrednet · 2025-11-26T13:50:23Z

Objects/dictobject.c

    if(!Py_CHECKWRITE(d)){
        PyErr_WriteToImmutable(d);
-        goto error;
+        return -1;


This should NULL the result pointer, as this is the defined behaviour on fail AFAIK. This can also use goto error; which will NULL the result.

Now it looks like this logic is split across multiple functions, most often this one, but for this specific case, there is also a case in PyDict_SetDefault()

I think this is probably a merging error. I will revert to the goto I think that fixes all the cases.

xFrednet · 2025-11-26T14:13:22Z

Python/immutability.c

+#ifdef MERMAID_TRACING
+#define TRACE_MERMAID_START() \
+    do { \
+        FILE* f = fopen("freeze_trace.md", "w"); \


Freeze calls can be nested due to reentry. will the cause problems, if the same file is opend and then closed twice? (Besides the output being no proper mermaid)

Good point. I think this is still useful. But perhaps needs more engineering. Should I drop from this PR, and make a separate PR for this?

xFrednet · 2025-11-28T15:51:25Z

Python/immutability.c

+        //  CHANGES HERE NEED TO BE REFLECTED IN freeze_visit
+
+        if (PyType_Check(c)) {
+            // TODO(Immutable): mjp: Special case for types not sure if required. We should review.


Likely required until we have a custom tp_reachable function for Pyrona.

This will likely also cover parts of the special case for weak references, making all of this code way cleaner :D

xFrednet · 2025-11-28T16:11:22Z

Python/immutability.c

-        // as we didn't change anything.
-        PyObject* item = pop(state->visited_list);
-        _Py_CLEAR_IMMUTABLE(item);
+    // tp_clear all elements in the cycle.


A comment to explain why this is safe would be good :)

Suggested change

// tp_clear all elements in the cycle.

// tp_clear all elements in the cycle.

// The elements will stay allocated since until `scc_return_to_gc` which will

// decrement the RC by one

xFrednet · 2025-11-28T16:12:37Z

Python/immutability.c

+    // // We need to add this attribute before traversing, so that if it creates a
+    // // dictionary, then this dictionary is frozen.
+    // if (state->freeze_location != NULL) {
+    //     // Some objects don't have attributes that can be set.
+    //     // As this is a Debug only feature, we could potentially increase the object
+    //     // size to allow this to be stored directly on the object.
+    //     if (PyObject_SetAttrString(obj, "__freeze_location__", state->freeze_location) < 0) {
+    //         // Ignore failure to set _freeze_location
+    //         PyErr_Clear();
+    //         // We still want to freeze the object, so we continue
+    //     }
+    // }


Should these be commented out?

Probably not, that was due to a failure which I didn't understand. And never reinstated.

xFrednet · 2025-12-01T13:14:24Z

Objects/typeobject.c

    if(!(tp->tp_getset == NULL ||
-         tp->tp_getset == subtype_getsets_full ||
-         tp->tp_getset == subtype_getsets_weakref_only ||
-         tp->tp_getset == subtype_getsets_dict_only))


What is the reason for these changes? o.O

python#136966

xFrednet · 2025-12-01T13:16:05Z

Python/immutability.c

-        // Object was not tracked in the GC, so we don't need to merge it back.
-        _PyGCHead_SET_PREV(gc, NULL);
-        _PyGCHead_SET_NEXT(gc, NULL);
+    Returns true if the SCC's reference count has become zero.


The function returns nothing, this comment is probably outdated?

Changes made in sprint for PLDI deadline.

451886c

Co-authored-by: xFrednet <xFrednet@gmail.com>

mjp41 force-pushed the immutable-pldi-rebased2 branch from 4b38c26 to 451886c Compare November 26, 2025 11:02

xFrednet self-requested a review November 26, 2025 14:08

xFrednet reviewed Dec 1, 2025

View reviewed changes

xFrednet force-pushed the immutable-pldi-rebased2 branch from 38630f0 to 451886c Compare December 1, 2025 18:14

mjp41 added 9 commits December 15, 2025 15:15

Addressing code review and CI failures.

f50d40c

Format

41c6bec

Windows build

d965acf

More CI fixes

2d6ce6f

More CI fixes

15be839

Remove free-threaded build until we do that work.

b758039

Fix yaml lint (maybe)

7f46141

Disable another free threaded build

cdd4161

Disable another free threaded build

684307d

mjp41 marked this pull request as ready for review December 17, 2025 10:49

mjp41 merged commit aa72f83 into immutable-main2 Dec 17, 2025
35 of 42 checks passed

mjp41 deleted the immutable-pldi-rebased2 branch December 17, 2025 10:49

		#define MAXFREELIST(x) x
		//#define MAXFREELIST(x) 0

Conversation

mjp41 commented Nov 26, 2025

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xFrednet Nov 26, 2025 •

edited

Loading