gh-121954: Don't mutate tuples in _PyCode_New

[encukou]

Change _PyCode_New (intern_strings & intern_constants) to return new tuples
rather than mutating their arguments.
If possible, return the argument rather than create a copy.

In order to make copying less common, intern strings destined for
_PyCode_New earlier -- in:

• _PyCode_ConstantKey, used in the compiler to de-duplicate constants;
• symtable_implicit_arg, which generates the names .0, .1 etc.
  • The most common implicit arg, .0, is baked in as a _Py_STR
  • others are immortalized

This means all string constants produced by the compiler are interned;
previously it was only names and identifier-like strings.
(We already hash all code constants in order to de-duplicate them,
and we treat deleting code objects as rare event. Also, constants were
interned in the free-threaded build.)
Remove a test that verified a string constant is not interned.

Merge validate_and_copy_tuple, the routine for cleaning up user input
for code.__new__, into the internal intern_names.
Note that this changes the exception type for non-string names
in PyCode_NewWithPosOnlyArgs and such from SystemError to TypeError;
IMO that doesn't need documenting.

Rename intern_strings to intern_names to further distinguish it from
intern_constants. Unlike constants, names are immortalized.

• Issue: Code object creation (const interning) can mutate tuples #121954

[encukou]

Copying the tuple should be a rare event, mostly for creating code from user data.
Should it use some kind of faster-cpython stat counter?

[carljm]

(Symtable change looks fine to me, would rather have someone from faster-cpython review the rest, once the PR is ready.)

[encukou]

Thanks!

(I'm aware of some of the problems in the PR; since it's not a 3.13 regression I'll get back to this after 3.13rc1 is out.)

[encukou]

I'm blocked on not understanding some of the internals for free-threaded builds.

The free-threaded build tries to “intern” all code constants, but it doesn't -- see for example this, adapted from test.test_code.CodeConstsTest but with a set added:

import textwrap


globals = {}
exec(textwrap.dedent("""
    def func1():
        return (0.0, (1, 2, "hello"), {1, 2})
"""), globals)

exec(textwrap.dedent("""
    def func2():
        return (0.0, (1, 2, "hello"), {1, 2})
"""), globals)

print(globals["func1"]() is globals["func2"]())

Which constants are meant to be interned here?

Worse, when a container (i.e. tuple) is interned, it's later cleared in the clear_containers function, replacing all its contents with NULL. This sounds dangerous for objects that might be shared with other code. What makes clear_containers safe?

@colesbury, do you know?

[colesbury]

The free-threaded build tries to “intern” all code constants, but it doesn't...

All types of code constants (frozenset, slices, strings, numbers) are interned, but there are some code paths that skip interning and immortalization:

• compile() doesn't intern or immortalize constants. One reason is so that the checked-in, frozen Python files match between the default and free-threaded build.
• We suppress the interning and immortalization in some tests, including in all refleak tests.

I think exec calls compile internally.

Worse, when a container (i.e. tuple) is interned, it's later cleared in the clear_containers function... This sounds dangerous for objects that might be shared with other code

We are deallocating the immortalized constants near the end of the interpreter shutdown process. Your comment is about clear_containers , but the hazards are with deallocating immortalized objects, and they're the same as with _PyUnicode_ClearInterned. If some (broken) extension holds onto a PyObject* across interpreter lifetimes, it will have an invalid pointer. But that's true in general because some mimalloc state is per-interpreter, and that state gets destroyed in finalize_interp_clear shortly after we free the constants.

The constants form an acyclic graph so we have to be careful about the order in which we free them. The emptying of containers breaks up this graph, which allows us to call _Py_ClearImmortal() in no particular order. That's not the only option; we could have implemented this differently:

• We could first topologically sort the constants and then call _Py_ClearImmortal() in topological order. That would ensure that we don't free an object while another constant still points to it. This seems more complicated to implement.
• We could do it in three passes over the constants. First, call _Py_SetMortal(op, 1) to set each object's refcount to 1. Second, traverse containers calling Py_INCREF() on pointed-to constants to fixup the reference counts. Finally, loop over constants and call Py_DECREF() once on each object.

[colesbury]

I'll take a closer look at the free-threading test failures

[colesbury]

@encukou, sorry I think the reported leaks were all pre-existing. They seem to be fixed by:

• gh-122697: Fix free-threading memory leaks at shutdown #122703

When I merge that PR into yours, all the tests pass. Would you please review that PR when you get a chance?

[encukou]

Thank you! That makes things much clearer.
Let me poke you with some questions to make sure we're on the same page:

All types of code constants (frozenset, slices, strings, numbers) are interned

Those are constants used by the compiler and marshal. But, users can put anything in co_consts, including mutable values or subtypes of core types.
When they do that, things will break, but I think it should not affect other code objects, or cause crashes or memory corruption.

For example:

def f():
    print(())

code = f.__code__
code = code.replace(co_consts=(None, []))
exec(code)

Would it make sense to limit this interning to the marshallable immutable types only?

frozenset

The frozenset code seems to rely on the order of elements, so it might intern two distinct but equal sets. Would that be a problem? (If so I'll try to find an example.)

We are deallocating the immortalized constants near the end of the interpreter shutdown process.

The danger I see is that the values interned here are assumed to be “owned” by this machinery, but it seems they could be shared with other code, and perhaps needed in some later stage of interpreter shutdown.
Or is there a mechanism that prevents, for example, the MRO tuple of a built-in class to find its way here?

[colesbury]

But, users can put anything in co_consts...

I'm concerned that allowing non-immortal or mutable types in constants will cause problems downstream in the interpreter. My preference would be to more thoroughly validate the replacement co_consts to ensure that they only contain the smae types the compiler produces.

it should not ... cause crashes or memory corruption...

This is generally our requirement for APIs, but code.replace is unsafe -- if the co_code is wrong the interpreter will crash horribly.

I still thinks it's a good goal -- the more footguns we remove, the better -- but I think we'd be better off if we get there by more thorough validation, rather than expanding scope/support.

The frozenset code seems to rely on the order of elements, so it might intern two distinct but equal sets.

I think the behavior matches logic in the bytecode compiler, which also de-dupes constants. We only de-dupe two sets if they contain the same elements and their iteration orders match. The iteration order requirement is not strictly necessary, but it simplifies the comparison logic.

...and perhaps needed in some later stage of interpreter shutdown

This happens very late in interpreter shutdown for that reason. The thread state and interpreter state are already cleared and static types have been finalized.

[encukou]

My preference would be to more thoroughly validate the replacement co_consts to ensure that they only contain the same types the compiler produces.

Sounds reasonable. I filed #122850, to not block this PR.

code.replace is unsafe -- if the co_code is wrong the interpreter will crash horribly.

Sure, but people could reasonably expect a “copy-and-patch” operation on co_consts to be safe. Let's fail cleanly rather than break the more obscure invariants.

This happens very late in interpreter shutdown for that reason. The thread state and interpreter state are already cleared and static types have been finalized.

I see. I'll add a comment to make it less likely for someone to add/rearrange the finalization phases.

(Leaving now; I'll update the PR next week.)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @encukou for commit ed6115f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[encukou]

Today, I wasn't able to figure out the refleak free-threaded buildbots found in test__interpchannels.
It goes away when I revert per-thread heap type refcounts (gh-122418). It also goes away when I revert this change (i.e. modify the parent tuple) for the one-element tuple ('encoding',).
I'll continue debugging later.

[colesbury]

@encukou let me know if you'd like me to take a look at the remaining refleak

[encukou]

Don't spend hours on it, but if there's a quick trick or tool you know of, or if can point to where you'd look next, I'd appreciate it.

[encukou]

This hides the refleak:

diff --git a/Lib/test/test__interpreters.py b/Lib/test/test__interpreters.py
index f493a92e0dd..9ffec59c8bf 100644
--- a/Lib/test/test__interpreters.py
+++ b/Lib/test/test__interpreters.py
@@ -26,7 +26,7 @@ def _captured_script(script):
     indented = script.replace('\n', '\n                ')
     wrapped = dedent(f"""
         import contextlib
-        with open({w}, 'w', encoding="utf-8") as spipe:
+        with open({w}, 'w', -1, "utf-8") as spipe:
             with contextlib.redirect_stdout(spipe):
                 {indented}
         """)