gh-132775: Add _PyObject_GetXIDataWithFallback()#133482

ericsnowcurrently · 2025-05-06T00:14:04Z

Issue: Improvements to test.support.interpreters.Interpreter #132775

neonene · 2025-05-08T19:11:15Z

Python/crossinterp.c

+#ifdef Py_DEBUG
+            Py_UNREACHABLE();


The #ifdef looks like a typo.

It's there to make sure we abort on debug builds, but we have a fallback for non-debug builds.

I'd prefer an explicit Py_FatalError() then. I think I can see this usage only in crossinterp.c (here, L1673, L1700).

My intention is to not crash under normal usage, which is what Py_FatalError() would do.

ah, Py_FatalError() instead of Py_UNREACHABLE()? I'm okay with that. It really should be unreachable through.

That's fair. I'll switch it to explicitly Py_FatalError().

Thanks. This is off-topic, but would one of the coming PRs address #133107 (comment)?

Yeah, I'll be addressing that.

neonene · 2025-05-13T22:27:40Z

Lib/test/test_crossinterp.py

+                EXCEPTION,
+                OBJECT,


nit: indent. Also, cls is undefined at L726, 732:

assert not hasattr(mod, func.__name__), (cls, getattr(mod, func.__name__))

neonene · 2025-05-13T22:28:20Z

Lib/test/test_crossinterp.py

+                # Currently the "extra" attrs are not preserved
+                # (via __reduce__).
+                self.assertIs(type(exc1), type(exc2))
+                #self.assert_exc_equal(grouped1, grouped2)


Is this still intentionally commented out? The assertion passed for me.

neonene · 2025-05-13T22:28:51Z

Lib/test/test_crossinterp.py

+    def assert_equal_or_equalish(self, obj, expected):
+        cls = type(expected)
+        if cls.__eq__ is not object.__eq__:
+#            assert cls not in (types.MethodType, types.BuiltinMethodType, types.MethodWrapperType), cls


Ditto. See also:

L757:

# self.assert_roundtrip_equal(defs.TOP_FUNCTIONS)

L844, 997, 1463:

# UnicodeError: (None, msg, None, None, None),

neonene · 2025-05-15T21:13:01Z

Python/crossinterp.c

+            Py_FatalError("unsupported xidata fallback option");
+#endif
+            _PyErr_SetString(tstate, PyExc_SystemError,
+                             "unsuppocted xidata fallback option");


nit: unsuppo"c"ted

neonene · 2025-05-15T23:12:28Z

Python/crossinterp.c

+}
+
+int
+_PyObject_GetXIDataWithFallback(PyThreadState *tstate,


Could we replace the long name with _PyObject_GetXIData? I feel the existing _PyObject_GetXIData could be longer (e.g. _PyObject_GetXIDataNoFallback), ~~as it is unpopular at all in newer PRs~~.

UPDATE: No, _PyObject_GetXIData should remain as-is.

I'm not sure whether the fallback parameter is needed. Does this function support the following?

call getdata.basic() under the _PyXIDATA_FULL_FALLBACK case

call getdata.fallback() under the _PyXIDATA_XIDATA_ONLY case

UPDATE: Yes, they are necessary.

I take back the questions in this topic. Sorry for the noise.

(I currently interpret getdata.fallback as getdata.(with)fallback that can pass an option to tuple's items, and WithFallback as _PyFunction_GetXIData and _PyPickle_GetXIData.)

Good point about the function name. I'll take a look.

As to the fallback parameter, it does not determine which function is called. The _PyXIData_getdata_t functions will be called for any of the fallback values. It's just a matter of if the fallback parameter is propagated to the getdata func.

I've changed the name as suggested. Thanks for that!

neonene · 2025-05-16T05:27:11Z

Include/internal/pycore_crossinterp.h

+typedef struct {
+    xidatafunc basic;
+    xidatafbfunc fallback;
+} _PyXIData_getdata_t;


Does the layout imply that it would accept two functions in the future? If exclusive, is there any reason why it cannot be a void pointer (or xidatafunc) with a flag? (The C compilers could verify xidatafbfunc at REGISTER_FALLBACK().)

E: No change request from me as long as there are only two options.

Yeah, I tried both and went with this because I don't expect other options. This is internal API so I wasn't going to worry about it too much for now. That said, maybe it would be better to take the void * approach now. I'll think about it.

I'm inclined to leave it the way it is, since I anticipate only those two options will be needed and it's nice to keep the signature explicit (instead of void *).

neonene · 2025-05-18T13:59:07Z

Include/internal/pycore_crossinterp.h

-typedef int (*xidatafunc)(PyThreadState *tstate, PyObject *, _PyXIData_t *);
+typedef int xidata_fallback_t;
+#define _PyXIDATA_XIDATA_ONLY (0)
+#define _PyXIDATA_FULL_FALLBACK (1)


Maybe flags like 0b111?

I added these with the idea that there is conceptual room for other options and the actual values aren't so important. However, they don't need to be unioned, so I'm not sure there's much value in using another literal form.

miss-islington-app · 2025-05-21T13:23:52Z

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

…-133482) It now supports a "full" fallback to _PyFunction_GetXIData() and then `_PyPickle_GetXIData()`. There's also room for other fallback modes if that later makes sense. (cherry picked from commit 88f8102) Co-authored-by: Eric Snow <[email protected]>

bedevere-app · 2025-05-21T13:24:04Z

GH-134418 is a backport of this pull request to the 3.14 branch.

It now supports a "full" fallback to _PyFunction_GetXIData() and then `_PyPickle_GetXIData()`. There's also room for other fallback modes if that later makes sense. (cherry picked from commit 88f8102, AKA gh-133482) Co-authored-by: Eric Snow <[email protected]>

…-133482) It now supports a "full" fallback to _PyFunction_GetXIData() and then `_PyPickle_GetXIData()`. There's also room for other fallback modes if that later makes sense.

ericsnowcurrently added the skip news label May 6, 2025

bedevere-app bot mentioned this pull request May 6, 2025

Improvements to test.support.interpreters.Interpreter #132775

Closed

ericsnowcurrently mentioned this pull request May 6, 2025

gh-132775: Use New Cross-Interpreter Internal C-API Functions #133483

Closed

ericsnowcurrently force-pushed the add-pyobject-getxidata-with-fallback branch from e72c2af to cf17342 Compare May 6, 2025 17:34

ericsnowcurrently added the needs backport to 3.14 bugs and security fixes label May 8, 2025

neonene reviewed May 8, 2025

View reviewed changes

ericsnowcurrently force-pushed the add-pyobject-getxidata-with-fallback branch 2 times, most recently from 04308b9 to b9f1eea Compare May 13, 2025 00:08

Add _PyObject_GetXIDataWithFallback().

55ccaa0

ericsnowcurrently force-pushed the add-pyobject-getxidata-with-fallback branch from b9f1eea to 55ccaa0 Compare May 13, 2025 00:46

ericsnowcurrently marked this pull request as ready for review May 13, 2025 00:47

bedevere-app bot added the awaiting core review label May 13, 2025

Py_UNREACHABLE() -> Py_FatalError().

d8b73ab

neonene reviewed May 13, 2025

View reviewed changes

neonene reviewed May 15, 2025

View reviewed changes

neonene reviewed May 16, 2025

View reviewed changes

neonene reviewed May 18, 2025

View reviewed changes

Fix typos.

8e017f8

ericsnowcurrently requested review from brettcannon, ncoghlan and warsaw as code owners May 20, 2025 17:00

ericsnowcurrently mentioned this pull request May 20, 2025

gh-132775: Add _PyPickle_GetXIData() #133107

Merged

ericsnowcurrently removed request for brettcannon, ncoghlan and warsaw May 20, 2025 17:56

_PyObject_GetXIDataWithFallback -> _PyObject_GetXIData

9693a5d

ericsnowcurrently merged commit 88f8102 into python:main May 21, 2025
38 checks passed

bedevere-app bot removed the awaiting core review label May 21, 2025

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label May 21, 2025

ericsnowcurrently deleted the add-pyobject-getxidata-with-fallback branch May 21, 2025 14:02

ericsnowcurrently mentioned this pull request May 23, 2025

Refleaks on free-threaded builds #134557

Closed

Uh oh!

Conversation

ericsnowcurrently commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

neonene May 15, 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!

neonene May 16, 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!

neonene May 16, 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!

Uh oh!

miss-islington-app bot commented May 21, 2025

Uh oh!

bedevere-app bot commented May 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericsnowcurrently commented May 6, 2025 •

edited

Loading

neonene May 15, 2025 •

edited

Loading

neonene May 16, 2025 •

edited

Loading

neonene May 16, 2025 •

edited

Loading