Make platform.h work on any libc with the standard rseq ABI - #141
Open
thevar1able wants to merge 2 commits into
Open
Make platform.h work on any libc with the standard rseq ABI#141thevar1able wants to merge 2 commits into
thevar1able wants to merge 2 commits into
Conversation
Selecting the GNU char *-returning strerror_r by feature-test macros misfires on libcs that accept _GNU_SOURCE but only ship the POSIX int-returning variant, breaking the build. Two strerrorResult overloads dispatch on the real return type instead, so silk::strerror works with either variant without preprocessor guessing.
getCurrentProcessor selected the rseq offset symbol at build time via __has_include(<sys/rseq.h>) and dereferenced cpu_id unconditionally, so behavior depended on the sysroot headers rather than the runtime libc, and an unregistered thread's cpu_id of -1 turned into an out-of-bounds shard index. The offset now always comes from librseq's rseq_offset, guarded by rseq_size cast to a signed value (librseq initializes it to -1U and sets it to 0 when the libc registration failed), and a negative cpu_id falls back to sched_getcpu clamped to shard 0. On glibc 2.35+ with rseq registered the fast path is unchanged apart from two predictable branches.
|
Workflow [PR], commit [ef15d39] Summary: ❌
Code ReviewResult: ✅ No issues found What changed: Updates platform utilities to select the libc's actual No correctness, concurrency, or performance issues found in the changed code. Investigation: 7/13 rounds, 44 tool calls. |
thevar1able
added a commit
to ClickHouse/ClickHouse
that referenced
this pull request
Aug 24, 2026
Silk's stack switching is boost fcontext (libc-agnostic); its libc coupling was confined to `platform.h` and is removed on the silk side by ClickHouse/silk#141. The `_silk_util` target force-includes the `__GNUC_PREREQ` shim because silk includes librseq headers. NOTE: musl builds of silk require the silk submodule to carry PR 141; the pointer bump follows once it merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vadimskipin
reviewed
Aug 24, 2026
|
|
||
| // glibc's sched_getcpu fast path is THREAD_GETMEM_VOLATILE(THREAD_SELF, rseq_area.cpu_id), | ||
| // which is the same rseq read we do here. We skip the function call and the cpu_id >= 0 | ||
| // fallback to vDSO/syscall -- safe on Linux 4.18+ / glibc 2.35+ where rseq is always registered. |
Collaborator
There was a problem hiding this comment.
The whole idea is to keep this function as tiny as possible. No, I do NOT want to have a callback here.
Collaborator
|
"On a thread whose rseq area is not registered yet" - how is this possible? |
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
2 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.
Needed for ClickHouse/ClickHouse#109239, which links ClickHouse statically against a different libc and enables silk in that build. Silk does not target any specific libc. It only requires a libc that registers rseq and exports
__rseq_offset/__rseq_size/__rseq_flags(glibc >= 2.35 does). Both commits change nothing on glibc >= 2.35.Commit 1: dispatch strerror_r on its actual return type. The old code picked the GNU
char *variant ofstrerror_rwhen_GNU_SOURCEis defined. Some libcs accept_GNU_SOURCEbut only provide the POSIXintvariant, so the build fails (18 TUs in the ClickHouse build). TwostrerrorResultoverloads now handle whichever variant the libc provides.Commit 2: read the rseq area through librseq's globals in getCurrentProcessor. Two problems with the old code:
__has_include(<sys/rseq.h>), so behavior depended on the sysroot headers, not on the libc the binary runs against.rseq->cpu_idwith no check. On a thread whose rseq area is not registered yet,cpu_idis -1, which cast touint16_tbecomes an out-of-bounds index into per-CPU state.Now the offset always comes from librseq's
rseq_offset, whichrseq_initcopies from the libc. The read is guarded byrseq_sizecast to signed (librseq sets it to-1Ubefore init and to 0 when libc registration failed). If the rseq read is not usable, we fall back tosched_getcpu, clamping errors to CPU 0 - the value is a sharding hint, so a wrong shard only costs contention. On glibc the fast path gains two predictable branches; checked in the disassembly.Tested: full suite passes on glibc (one pre-existing flaky test,
StealTest.computeFibersNeverSharePhysicalCore, fails the same way on main under parallel ctest load). In the ClickHouse static build,_silk_fibersand_silk_utilnow compile cleanly.