Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.12] gh-119311: Fix name mangling with PEP 695 generic classes (#119464) #119644

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 28, 2024

Fixes #119311. Fixes #119395.

(cherry picked from commit a9a74da)

@JelleZijlstra
Copy link
Member Author

"Check if the ABI has changed" is failing, this is what I was worried about. I'll try to think of a solution that doesn't involve new entries on the struct.

@carljm
Copy link
Member

carljm commented May 28, 2024

My recollection (admittedly vague) is that the ABI dump doesn't try to distinguish internal ABI, so an ABI check failure isn't a definitive no-go, it just means RM should take a look.

If we are concerned that someone might be building with Py_BUILD_CORE and depending on offsets into internal symtable entry structs, an easy change that would eliminate that concern would be to move the new field to the end of the struct in the 3.12 version. That would still require regenerating the ABI dump to pass CI (and RM should still be consulted) but would mean nobody accessing existing fields would be broken.

@JelleZijlstra
Copy link
Member Author

@Yhg1s what do you think of merging this into 3.12? It fixes two bugs with name mangling around PEP 695 (#119311, #119395), but makes some changes to the internal C API to do so.

@Yhg1s
Copy link
Member

Yhg1s commented Jun 4, 2024

Yes, I think it makes sense to backport this to 3.12. The ABI breakage doesn't matter because the structs aren't visible as Carl said, and the new function is fine because it's private and not used by extension modules, just the core itself.

You do need to regen the ABI dump as part of the PR.

@JelleZijlstra
Copy link
Member Author

I couldn't get the abi regen tool to run locally (I tried the instructions in https://devguide.python.org/getting-started/setup-building/#regenerate-the-abi-dump but got errors related to mpdecimal). But I was able to edit the dump file manually and the check succeeds now.

@Yhg1s Yhg1s enabled auto-merge (squash) June 4, 2024 19:55
@Yhg1s Yhg1s merged commit dc40226 into python:3.12 Jun 4, 2024
27 checks passed
@JelleZijlstra JelleZijlstra deleted the backport-a9a74da-3.12 branch June 4, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants