Skip to content

gh-130115: fix thread identifiers for 32-bit musl #130391

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Apr 4, 2025
Merged

gh-130115: fix thread identifiers for 32-bit musl #130391

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
51d6579
Return the main thread's short identifier on certain platforms
vfazio Feb 21, 2025
a9ca268
Add NEWS entry
vfazio Feb 21, 2025
f6ad25d
Merge branch 'main' into vfazio-thread_get_ident
vfazio Feb 21, 2025
b93868e
Merge remote-tracking branch 'upstream/main' into vfazio-thread_get_i…
vfazio Feb 23, 2025
7a4de5f
Move pthread_t to ident conversion to a shared function
vfazio Feb 23, 2025
af1233b
Reword NEWS entry
vfazio Feb 23, 2025
7bdfd34
Revert "Return the main thread's short identifier on certain platforms"
vfazio Feb 23, 2025
dc7c497
Merge remote-tracking branch 'upstream/main' into vfazio-thread_get_i…
vfazio Feb 23, 2025
140a4d8
simplify conversion logic
vfazio Apr 2, 2025
63812b5
Merge remote-tracking branch 'upstream/main' into vfazio-thread_get_i…
vfazio Apr 2, 2025
4ae6d7e
Merge branch 'main' into vfazio-thread_get_ident
vfazio Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-02-21-00-12-24.gh-issue-130115.mF-rP6.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix an issue with thread identifiers being sign-extended on some platforms.
31 changes: 22 additions & 9 deletions Python/thread_pthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,16 +306,34 @@ do_start_joinable_thread(void (*func)(void *), void *arg, pthread_t* out_id)
return 0;
}

/* Helper to convert pthread_t to PyThread_ident_t. POSIX allows pthread_t to be
non-arithmetic, e.g., musl typedefs it as a pointer */
static PyThread_ident_t
_pthread_t_to_ident(pthread_t value) {
#if SIZEOF_PTHREAD_T > SIZEOF_LONG
return (PyThread_ident_t) *(unsigned long *) &value;
Copy link
Member

@pitrou pitrou Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why go through a pointer? This is cryptic and also uncomfortably fragile (what about big-endian platforms?).

I would suggest perhaps something like:

static PyThread_ident_t
_pthread_t_to_ident(pthread_t value) {
    // Avoid sign-extension when converting to a larger int type
#if SIZEOF_PTHREAD_T == SIZEOF_VOID_P
    return (uintptr_t) value;
#elif SIZEOF_PTHREAD_T == SIZEOF_LONG
    return (unsigned long) value;
#elif SIZEOF_PTHREAD_T == SIZEOF_INT
    return (unsigned int) value;
#elif SIZEOF_PTHREAD_T == SIZEOF_LONG_LONG
    return (unsigned long long) value;
#else
#error "Unsupported SIZEOF_PTHREAD_T value"
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the branch to this implementation. The unit tests continue to pass AFAICT.

#else
PyThread_ident_t ident;
#if defined(__linux__) && !defined(__GLIBC__)
ident = (PyThread_ident_t) (uintptr_t) value;
assert(pthread_equal(value, (pthread_t) (uintptr_t) ident));
#else
ident = (PyThread_ident_t) value;
assert(pthread_equal(value, (pthread_t) ident));
#endif
return ident;
#endif // SIZEOF_PTHREAD_T > SIZEOF_LONG
}

int
PyThread_start_joinable_thread(void (*func)(void *), void *arg,
PyThread_ident_t* ident, PyThread_handle_t* handle) {
pthread_t th = (pthread_t) 0;
if (do_start_joinable_thread(func, arg, &th)) {
return -1;
}
*ident = (PyThread_ident_t) th;
*ident = _pthread_t_to_ident(th);
*handle = (PyThread_handle_t) th;
assert(th == (pthread_t) *ident);
assert(th == (pthread_t) *handle);
return 0;
}
Expand All @@ -328,11 +346,7 @@ PyThread_start_new_thread(void (*func)(void *), void *arg)
return PYTHREAD_INVALID_THREAD_ID;
}
pthread_detach(th);
#if SIZEOF_PTHREAD_T <= SIZEOF_LONG
return (unsigned long) th;
#else
return (unsigned long) *(unsigned long *) &th;
#endif
return (unsigned long) _pthread_t_to_ident(th);;
}

int
Expand All @@ -357,8 +371,7 @@ PyThread_get_thread_ident_ex(void) {
if (!initialized)
PyThread_init_thread();
threadid = pthread_self();
assert(threadid == (pthread_t) (PyThread_ident_t) threadid);
return (PyThread_ident_t) threadid;
return _pthread_t_to_ident(threadid);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vfazio vfazio Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#if SIZEOF_PTHREAD_T <= SIZEOF_LONG
    return (unsigned long) th;
#else
    return (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:

2565bff

635f6fb

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:

static PyThread_ident_t
_pthread_t_to_ident(pthread_t value) {
    PyThread_ident_t ident;
#if defined(__linux__) && !defined(__GLIBC__)
    ident = (PyThread_ident_t) (uintptr_t) value;
    assert(pthread_equal(value, (pthread_t) (uintptr_t) ident));
#else
    ident = (PyThread_ident_t) value;
    assert(pthread_equal(value, (pthread_t) ident));
#endif
    return ident;
}

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.

}

unsigned long
Expand Down
Loading