-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196
base: main
Are you sure you want to change the base?
Conversation
corona10
commented
Dec 23, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Race in PyUnicode_InternFromString under free-threading #128137
@colesbury I am not sure what you intended. |
f30b355 breaks WASI test: https://github.com/python/cpython/actions/runs/12469477403/job/34802702354?pr=128196 (object size is increased). |
Include/cpython/unicodeobject.h
Outdated
3: Interned, Immortal, and Static | ||
This categorization allows the runtime to determine the right | ||
cleanup mechanism at runtime shutdown. */ | ||
uint8_t interned; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this remain in the state
struct. It's okay for a struct to contain both non-bitfield and bitfield members:
- It avoids a potential unnecessary breakage from moving the field
- Keeping it in
state
will make it easier to keepstate
32-bits due to alignment.
I think this should have a NEWS entry given that we're changing the size of a semi-public field. |
Since we touch the semi-public field, I am not sure about backporting this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming the tests pass
I agree -- I don't think we should backport this. |
There looks like issue with Win32, I will see tomorrow :) |
cc @methane |
610896e
to
b1b396b
Compare
I am now debugging on my Windows machine.. |
I 've updated |