gh-130115: fix thread identifiers for 32-bit musl#130391
Merged
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
CPython's pthread-based thread identifier relies on pthread_t being able to be represented as an unsigned integer type. This is true in most Linux libc implementations where it's defined as an unsigned long, however musl typedefs it as a struct *. If the pointer has the high bit set and is cast to PyThread_ident_t, the resultant value can be sign-extended [0]. This can cause issues when comparing against threading._MainThread's identifier. The main thread's identifier value is retrieved via _get_main_thread_ident which is backed by an unsigned long which truncates sign extended bits. >>> hex(threading.main_thread().ident) '0xb6f33f3c' >>> hex(threading.current_thread().ident) '0xffffffffb6f33f3c' Work around this by conditionally compiling in some code for non-glibc based Linux platforms that are at risk of sign-extension to return a PyLong based on the main thread's unsigned long thread identifier if the current thread is the main thread. [0]: https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Arrays-and-pointers-implementation.html Signed-off-by: Vincent Fazio <vfazio@gmail.com>
Signed-off-by: Vincent Fazio <vfazio@gmail.com>
ContributorAuthor
vfazio commented Feb 22, 2025
Drafting this. After doing some initial testing, changing only
I plan on testing on actual hardware (RPi on 32bit Alpine) at some point, but I've emulated this environment in Docker using arm32v7/alpine & the qemu user space emulator in the mean time. I'll rework the patch and then validate on hardware as well |
Move this to a static function so all callers have consistent behavior. Signed-off-by: Vincent Fazio <vfazio@gmail.com>
vfazio changed the title Signed-off-by: Vincent Fazio <vfazio@gmail.com>
ContributorAuthor
I built python I then edited the ident function to always set the upper bit to force the broken behavior I was seeing in #130115 and re-ran the tests |
ContributorAuthor
vfazio commented Feb 23, 2025
I still want to test on physical hardware and not on an emulated stack, but I have more confidence in this solution. |
ContributorAuthor
vfazio commented Feb 23, 2025
forgot to revert the initial commit. Test still pass on a |
ContributorAuthor
vfazio commented Feb 24, 2025
tested on hardware: |
ContributorAuthor
vfazio commented Feb 24, 2025
repeated with high bit set: |
ContributorAuthor
vfazio commented Mar 31, 2025
@mpage It's been a while since we discussed #130115 so I'm interested in your thoughts here. This PR addresses the primary issue, can be backported, and, as far as I can tell, doesn't introduce any regressions. I took a peek at some other changes last night and I'm curious if you think it would be worthwhile to reuse a similar data structure as the min heap for thread identifiers long term. It would recycle them faster, but so long as callers aren't assuming a specific lifetime maybe that's OK. I guess other assumptions are that thread IDs are tied to a specific interpreter instance and can't be passed around between them since they're no longer system level identifiers |
mpage reviewed Mar 31, 2025
Contributor
mpage left a comment
mpage left a comment There was a problem hiding this comment.
This looks mostly ok to me, but the use of _pthread_t_to_ident in PyThread_get_thread_ident_ex() makes me a little nervous when SIZEOF_PTHREAD_T > SIZEOF_LONG (see inline comment). I would probably take the approach of adding a second field to PyRuntime (even though it's gross) or potentially reverting to the previous behavior of using set_ident for the main thread. I'd like to see what others think. I've added a couple of folks who have worked in this area in the past as reviewers.
| threadid=pthread_self(); | ||
| assert(threadid== (pthread_t) (PyThread_ident_t) threadid); | ||
| return (PyThread_ident_t) threadid; | ||
| return_pthread_t_to_ident(threadid); |
Contributor
There was a problem hiding this comment.
I think this is potentially lossy compared to the previous version if SIZEOF_PTHREAD_T > SIZEOF_LONG. Previously we would cast directly to a PyThread_ident_t (an unsigned long long), whereas we now cast through an unsigned long.
ContributorAuthor
There was a problem hiding this comment.
I think I understand your concern.
This was an edge case in PyThread_start_new_thread, my guess is that it was there for the situations where pthread_t was not a ulong and was, instead, a pointer or struct.
#ifSIZEOF_PTHREAD_T <= SIZEOF_LONGreturn (unsigned long) th; #elsereturn (unsigned long) *(unsigned long*) &th;It's probably fine to drop the condition in _pthread_t_to_ident and let the caller here truncate it since this API is specifically returning a ULONG and not a PyThread_ident_t.
Looking at the history:
The cast through ulong* was for Alpha OSF which was dropped from PEP 11 a few years ago (CPython 3.3) https://bugs.python.org/issue8606. My guess is it was also defined as a pointer or struct type and this was a way to work around it by returning at least long bytes.
I think if we find other platforms where we need to support this workaround, they can be chained to the MUSL #ifdef I think. The only time it would maybe be a problem is if sizeof(uintptr_t) < sizeof(ulong).
If others agree, I can drop the condition so the function looks like so:
staticPyThread_ident_t_pthread_t_to_ident(pthread_tvalue){PyThread_ident_tident; #if defined(__linux__) && !defined(__GLIBC__) ident= (PyThread_ident_t) (uintptr_t) value; assert(pthread_equal(value, (pthread_t) (uintptr_t) ident)); #elseident= (PyThread_ident_t) value; assert(pthread_equal(value, (pthread_t) ident)); #endifreturnident}I do not suggest this as a long term solution; I do think we need to work towards making this opaque. I'm just trying to find something that is a stop-gap that can be ported back with relative ease that doesn't cause a regression.
Contributor
mpage commented Mar 31, 2025
Yes, something like that could make sense.
I think that is already true now (you can't make assumptions about the lifetime of pthread identifiers).
Yeah, depending on how we chose to manage thread IDs that could be true. We could choose to manage the the pool of thread IDs per runtime, which would preserve the existing namespacing. |
pitrou reviewed Apr 1, 2025
Python/thread_pthread.h Outdated
| staticPyThread_ident_t | ||
| _pthread_t_to_ident(pthread_tvalue){ | ||
| #ifSIZEOF_PTHREAD_T>SIZEOF_LONG | ||
| return (PyThread_ident_t) *(unsigned long*) &value; |
Member
There was a problem hiding this comment.
Why go through a pointer? This is cryptic and also uncomfortably fragile (what about big-endian platforms?).
I would suggest perhaps something like:
staticPyThread_ident_t_pthread_t_to_ident(pthread_tvalue){// Avoid sign-extension when converting to a larger int type#ifSIZEOF_PTHREAD_T==SIZEOF_VOID_Preturn (uintptr_t) value; #elifSIZEOF_PTHREAD_T==SIZEOF_LONGreturn (unsigned long) value; #elifSIZEOF_PTHREAD_T==SIZEOF_INTreturn (unsigned int) value; #elifSIZEOF_PTHREAD_T==SIZEOF_LONG_LONGreturn (unsigned long long) value; #else#error "Unsupported SIZEOF_PTHREAD_T value" #endif }ContributorAuthor
There was a problem hiding this comment.
That was legacy code that I can certainly drop (as mentioned above) . This is definitely simpler, I'll give it a spin and push it if tests pass.
ContributorAuthor
There was a problem hiding this comment.
I've updated the branch to this implementation. The unit tests continue to pass AFAICT.
ContributorAuthor
Testing on musl + armv7 on RPi passes with the implementation suggested with and without the high bit set |
pitrou approved these changes Apr 3, 2025
Member
pitrou left a comment
pitrou left a comment There was a problem hiding this comment.
+1, LGTM, but we should run some extended CI checks on this
pitrou added the 🔨 test-with-buildbots 
bedevere-bot commented Apr 3, 2025
🤖 New build scheduled with the buildbot fleet by @pitrou for commit 4ae6d7e 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130391%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot removed the 🔨 test-with-buildbots 
Member
pitrou commented Apr 3, 2025
@gpshead Do you think this is a candidate for backporting? |
ContributorAuthor
I'm secretly hoping so, I plan on porting the patch for Buildroot, but backporting to 3.13 would allow BR to drop it once we hit 3.13.3 hopefully? |
pitrou merged commit 7212306 into python:main 126 of 129 checks passed
Uh oh!
There was an error while loading. Please reload this page.
pitrou added needs backport to 3.12 
Sorry, @vfazio and @pitrou, I could not cleanly backport this to |
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 4, 2025
) CPython's pthread-based thread identifier relies on pthread_t being able to be represented as an unsigned integer type. This is true in most Linux libc implementations where it's defined as an unsigned long, however musl typedefs it as a struct *. If the pointer has the high bit set and is cast to PyThread_ident_t, the resultant value can be sign-extended [0]. This can cause issues when comparing against threading._MainThread's identifier. The main thread's identifier value is retrieved via _get_main_thread_ident which is backed by an unsigned long which truncates sign extended bits. >>> hex(threading.main_thread().ident) '0xb6f33f3c' >>> hex(threading.current_thread().ident) '0xffffffffb6f33f3c' Work around this by conditionally compiling in some code for non-glibc based Linux platforms that are at risk of sign-extension to return a PyLong based on the main thread's unsigned long thread identifier if the current thread is the main thread. [0]: https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Arrays-and-pointers-implementation.html --------- (cherry picked from commit 7212306) Co-authored-by: Vincent Fazio <vfazio@gmail.com> Signed-off-by: Vincent Fazio <vfazio@gmail.com>
GH-132089 is a backport of this pull request to the 3.13 branch. |
Member
The threading code base has probably diverged too much from 3.12 to make an automatic backport feasible. You may want to backport an equivalent fix manually @vfazio , or we can limit ourselves to 3.13 if you prefer. |
ContributorAuthor
vfazio commented Apr 4, 2025
Thanks for porting this back! I think back to 3.13 is fine since that's when we first noticed the problem. It was probably an artifact of the change from UL -> ULL ( |
pitrou pushed a commit that referenced this pull request Apr 4, 2025
…H-132089) CPython's pthread-based thread identifier relies on pthread_t being able to be represented as an unsigned integer type. This is true in most Linux libc implementations where it's defined as an unsigned long, however musl typedefs it as a struct *. If the pointer has the high bit set and is cast to PyThread_ident_t, the resultant value can be sign-extended [0]. This can cause issues when comparing against threading._MainThread's identifier. The main thread's identifier value is retrieved via _get_main_thread_ident which is backed by an unsigned long which truncates sign extended bits. >>> hex(threading.main_thread().ident) '0xb6f33f3c' >>> hex(threading.current_thread().ident) '0xffffffffb6f33f3c' Work around this by conditionally compiling in some code for non-glibc based Linux platforms that are at risk of sign-extension to return a PyLong based on the main thread's unsigned long thread identifier if the current thread is the main thread. [0]: https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Arrays-and-pointers-implementation.html --------- (cherry picked from commit 7212306) Signed-off-by: Vincent Fazio <vfazio@gmail.com> Co-authored-by: Vincent Fazio <vfazio@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CPython's pthread-based thread identifier relies on pthread_t being able to be represented as an unsigned integer type.
This is true in most Linux libc implementations where it's defined as an unsigned long, however musl typedefs it as a struct *.
If the pointer has the high bit set and is cast to PyThread_ident_t, the resultant value can be sign-extended (https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Arrays-and-pointers-implementation.html). This can cause issues when comparing against
threading._MainThread's identifier. The main thread's identifier value is retrieved via_get_main_thread_identwhich is backed by an unsigned long which truncates sign extended bits.Work around this by making a new function which translates a
pthread_ttoPyThread_ident_tby casting through an integer type of the appropriate size to avoid sign extension. Factoring this out allows us to replace the internal logic in the future with some min-heap or other data structure to managepthread_tobjects in a more opaque fashion.Note musl isn't "officially" supported in PEP 11, however platform detection was added in c163d7f and similar PRs have been merged in the past which target it 5633c4f
This PR is intended to be a "minimum" to get this working. Longer term there should maybe be work to keep
pthread_topaque and not make assumptions about its type.