gh-125286: Bug fix for trace-refs logic and shared objects

[nascheme]

Fix the TraceRefs build to handle interned strings (both mortal and immortal) that are shared between interpreters.

• Issue: test_audit_subinterpreter crashes with tracerefs enabled #125286

[nascheme]

The TSAN nogil build fails tests for me on the main branch as well. So I'm not sure this change is the cause of the test failure. I get these failures on main branch:

8 tests failed:
    test_capi.test_mem test_capi.test_pyatomic test_functools
    test_importlib test_queue test_signal test_threading_local
    test_threadsignals

[ericsnowcurrently]

I'm a little unclear on the change here. My understanding is that we don't want the interned strings on legacy subinterpreters to be tracked on those interpreter's refs. It seems to me like the change in this PR has stopped fixing that for each interpreter, which would put us back to where we were before. What am I missing?

Other than that, I have one small suggestion.

[ericsnowcurrently]

FYI, you can also use _Py_IsMainInterpreter():

Suggested change

	PyInterpreterState *main_interp = _PyInterpreterState_Main();
	if (interp != main_interp &&
	interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) {
	if (_Py_IsMainInterpreter(interp) &&
	interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) {

[nascheme]

I'm a little unclear on the change here. My understanding is that we don't want the interned strings on legacy subinterpreters to be tracked on those interpreter's refs. It seems to me like the change in this PR has stopped fixing that for each interpreter, which would put us back to where we were before. What am I missing?

A problem with trace refs occurs both with immortal and mortal interned strings. The previous fix, _Py_NormalizeImmortalReference, only handled the immortal case. The new test I added triggers both cases and if you remove either fix, it crashes in _Py_ForgetReference.

I was trying to trigger a third case, where objects other than interned strings are shared and are created in one interpreter and deallocated in a different one. That would cause _Py_ForgetReference to fail too. However, I can't seem to make it happen, I think because of the m_copy dict stored in runtime structure. So, maybe we are safe from that one.

Other than that, I have one small suggestion.

Fixed as you suggest.

[nascheme]

Maybe I overdid it with the comments, some are a bit redundant. Another idea I was toying with is to make this tracerefs cleanup logic active only if non-isolated interpreters are being used. You need a flag on the main interp to know if sub-interpreters have been used that share its interned string dict. Possible patch here:

https://gist.github.com/nascheme/2243a137b9f9aa4639590231e3b8e004

It adds a new set of internal flags to the interp state so I think it can't be backported since it changes the ABI. I tried using feature_flags but I think that's not the correct place to put the flag. It's not a public API.

[encukou]

I wonder: for interpreters that use_main_obmalloc, would it make sense to also use the main ref chain?

[nascheme]

I wonder: for interpreters that use_main_obmalloc, would it make sense to also use the main ref chain?

I think that could work and would simplify the code. The major impact would be that gc.getobjects() in one of those interpreters (could be main) would return objects for all sub-interpreters with that flag. Maybe that's an improvement actually. I'm not sure what people would expect. Ask @pablogsal maybe since he uses the TraceRefs build?

[pablogsal]

I think that the assumption so far of that call is to get all objects in existence. I don't think semantics have been defined after subinterpeters were introduced so we have some wiggle room here

[ericsnowcurrently]

Here's a PR that shares the main refchain with all legacy interpreters: #125709. FWIW, they all used to share it when it was stored in global variables.

[nascheme]

Closing since GH-125709 fixes the same issue and seems a better approach.