NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

gh-132775: Add _PyObject_GetXIDataWithFallback()#133482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ericsnowcurrently merged 4 commits intopython:mainfromericsnowcurrently:add-pyobject-getxidata-with-fallback

May 21, 2025

Merged

gh-132775: Add _PyObject_GetXIDataWithFallback()#133482

ericsnowcurrently merged 4 commits intopython:mainfromericsnowcurrently:add-pyobject-getxidata-with-fallback

May 21, 2025

+581 −139

Conversation

Copy link

Member

ericsnowcurrently commentedMay 6, 2025•
edited
Loading

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

ericsnowcurrently added the skip news label

May 6, 2025

bedevere-appbot mentioned this pull request

May 6, 2025

Improvements to test.support.interpreters.Interpreter#132775

Open

ericsnowcurrently mentioned this pull request

May 6, 2025

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

Closed

ericsnowcurrently force-pushed theadd-pyobject-getxidata-with-fallback branch frome72c2af tocf17342Compare

May 6, 2025 17:34

ericsnowcurrently added the needs backport to 3.14bugs and security fixes label

May 8, 2025

neonene reviewed

May 8, 2025

View reviewed changes

Python/crossinterp.c Outdated

Comment on lines 481 to 482

		#ifdef Py_DEBUG
		Py_UNREACHABLE();

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The#ifdef looks like a typo.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd prefer an explicitPy_FatalError() then. I think I can see this usage only incrossinterp.c (here, L1673, L1700).

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, I'll be addressing that.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

ericsnowcurrently force-pushed theadd-pyobject-getxidata-with-fallback branch 2 times, most recently from04308b9 tob9f1eeaCompare

May 13, 2025 00:08

Add _PyObject_GetXIDataWithFallback().

55ccaa0

ericsnowcurrently force-pushed theadd-pyobject-getxidata-with-fallback branch fromb9f1eea to55ccaa0Compare

May 13, 2025 00:46

ericsnowcurrently marked this pull request as ready for review

May 13, 2025 00:47

bedevere-appbot added the awaiting core review label

May 13, 2025

Py_UNREACHABLE() -> Py_FatalError().

d8b73ab

neonene reviewed

May 13, 2025

View reviewed changes

Lib/test/test_crossinterp.py Outdated

Comment on lines 1342 to 1343

		EXCEPTION,
		OBJECT,

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

assertnothasattr(mod,func.__name__), (cls,getattr(mod,func.__name__))

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

Lib/test/test_crossinterp.py Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

Lib/test/test_crossinterp.py Outdated

		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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ditto. See also:

L757:

#        self.assert_roundtrip_equal(defs.TOP_FUNCTIONS)

L844, 997, 1463:

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

neonene reviewed

May 15, 2025

View reviewed changes

Python/crossinterp.c Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: unsuppo"c"ted

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

neonene reviewed

May 15, 2025

View reviewed changes

Python/crossinterp.c Outdated

		}

		int
		_PyObject_GetXIDataWithFallback(PyThreadState*tstate,

Copy link

Contributor

neoneneMay 15, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

neoneneMay 16, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

callgetdata.basic() under the_PyXIDATA_FULL_FALLBACK case
callgetdata.fallback() under the_PyXIDATA_XIDATA_ONLY case

UPDATE: Yes, they are necessary.

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

(I currently interpretgetdata.fallback asgetdata.(with)fallback that can pass an option to tuple's items, andWithFallback as_PyFunction_GetXIData and_PyPickle_GetXIData.)

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

ericsnowcurrentlyMay 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

neonene reviewed

May 16, 2025

View reviewed changes

Include/internal/pycore_crossinterp.h

Comment on lines +147 to +150

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

Copy link

Contributor

neoneneMay 16, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 verifyxidatafbfunc atREGISTER_FALLBACK().)

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

Copy link

MemberAuthor

ericsnowcurrentlyMay 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

ericsnowcurrentlyMay 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 ofvoid *).

neonene reviewed

May 18, 2025

View reviewed changes

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)

Copy link

Contributor

neoneneMay 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe flags like 0b111?

Copy link

MemberAuthor

ericsnowcurrentlyMay 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Fix typos.

8e017f8

ericsnowcurrently requested review frombrettcannon,ncoghlan andwarsaw ascode owners

May 20, 2025 17:00

ericsnowcurrently mentioned this pull request

May 20, 2025

gh-132775: Add _PyPickle_GetXIData()#133107

Merged

ericsnowcurrently removed request forbrettcannon,warsaw andncoghlan

May 20, 2025 17:56

_PyObject_GetXIDataWithFallback -> _PyObject_GetXIData

9693a5d

ericsnowcurrently merged commit88f8102 intopython:main

May 21, 2025

38 checks passed

bedevere-appbot removed the awaiting core review label

May 21, 2025

Copy link

miss-islington-appbot commentedMay 21, 2025

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

May 21, 2025

pythongh-132775: Support Fallbacks in _PyObject_GetXIData() (pythongh…

50e1340

…-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 commit88f8102)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Copy link

bedevere-appbot commentedMay 21, 2025

GH-134418 is a backport of this pull request to the3.14 branch.

bedevere-appbot removed the needs backport to 3.14bugs and security fixes label

May 21, 2025

ericsnowcurrently deleted the add-pyobject-getxidata-with-fallback branch

May 21, 2025 14:02

ericsnowcurrently added a commit that referenced this pull request

May 21, 2025

[3.14]gh-132775: Support Fallbacks in _PyObject_GetXIData() (gh-134418)

2ffc10b

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 commit88f8102, AKAgh-133482)Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ericsnowcurrently mentioned this pull request

May 23, 2025

Refleaks on free-threaded builds#134557

Open

Labels

skip news

2 participants

Movatterモバイル変換

Uh oh!

gh-132775: Add _PyObject_GetXIDataWithFallback()#133482

gh-132775: Add _PyObject_GetXIDataWithFallback()#133482

Conversation

ericsnowcurrently commentedMay 6, 2025• editedLoading 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!

neoneneMay 15, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neoneneMay 16, 2025• editedLoading 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!

neoneneMay 16, 2025• editedLoading 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-appbot commentedMay 21, 2025

Uh oh!

bedevere-appbot commentedMay 21, 2025

Uh oh!

Uh oh!

ericsnowcurrently commentedMay 6, 2025•
edited
Loading

neoneneMay 15, 2025•
edited
Loading

neoneneMay 16, 2025•
edited
Loading

neoneneMay 16, 2025•
edited
Loading