[6.2] Fix worker thread name handling on Linux#935
Open
KushalP wants to merge 5 commits intoswiftlang:release/6.2from
Open
[6.2] Fix worker thread name handling on Linux#935KushalP wants to merge 5 commits intoswiftlang:release/6.2from
KushalP wants to merge 5 commits intoswiftlang:release/6.2from
Conversation
**Explanation:** The existing functionality is brittle on Linux, and the UX can be improved. **Scope:** Narrow. Hard codes the thread name for queue workers. **Original PRs:** swiftlang#934 **Risk:** Very low. **Testing:** CI
Contributor
Author
|
@swift-ci please test. |
Contributor
Author
|
@swift-ci Please test Windows platform. |
## Problem PR swiftlang#934 (fd153d8) adds a `dispatch_thread_name` regression test that asserts worker threads have a non-empty name. This test fails on Amazon Linux 2 CI because `HAVE_PTHREAD_SETNAME_NP` is never set. The root cause is CMake's `check_function_exists` attempting to link a test program without `-lpthread`. On glibc < 2.34 (Amazon Linux 2 ships glibc 2.26), `pthread_setname_np` lives in a separate `libpthread.so`[1]. The link fails, `HAVE_PTHREAD_SETNAME_NP` evaluates to false, and the `pthread_setname_np` call in `queue.c` is compiled out. Worker threads get no name, and the test assertion `strlen(thread_name) > 0` fails. ## Fix Replace `check_function_exists` with `check_symbol_exists` against `<pthread.h>`, and temporarily add `Threads::Threads` to `CMAKE_REQUIRED_LIBRARIES` so the linker can resolve it. This matches the pattern used by `llvm/cmake/config-ix.cmake`[2]. Guard the `dispatch_thread_name` test on `HAVE_PTHREAD_SETNAME_NP` so it is only built when available. ## References [1] https://github.com/bminor/glibc/blob/glibc-2.34/NEWS#L12-L16 [2] https://github.com/swiftlang/llvm-project/blob/8050c17d40a6bcf781c3fdf7120b8f2d3c6363ac/llvm/cmake/config-ix.cmake#L434-L443
Contributor
Author
|
@swift-ci please test. |
Contributor
Author
|
@swift-ci please test. |
Member
|
Please ensure that this works cross-platform before merging this (FreeBSD and OpenBSD are both running into issues with it on |
Contributor
Author
Contributor
Author
|
Do not merge until #937 is merged to |
Here we just do this by token swapping, which feels a little dirty, but fixing it otherwise would induce a larger diff to factor out and switch on the pthread_get_name_np spelling variations. Fixing this requires making setting the thread name non-Linux specific. This is safe, because we've added cmake conditionals for the symbols.
Contributor
Author
|
@swift-ci please test. |
Co-authored-by: Alastair Houghton <alastair@alastairs-place.net>
Contributor
Author
|
@swift-ci please test. |
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
Sign up for free
to 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.
Explanation: The existing functionality is brittle on Linux, and the UX can be improved.
Scope: Narrow. Hard codes the thread name for queue workers.
Original PRs:
pthread_setname_npdetection on Amazon Linux 2 #938Risk: Very low.
Testing: CI