Implement sys/thread/vibix.rs and sys/sync/vibix/ (futex-based) in std fork#869
Implement sys/thread/vibix.rs and sys/sync/vibix/ (futex-based) in std fork#869
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis pull request adds futex and thread support for the Vibix platform to Rust's standard library. It introduces low-level futex syscall wrappers in userspace ABI, implements a Vibix-specific thread spawning and joining mechanism via Linux-style clone and futex, and wires the Vibix target into the existing futex-based implementations of Mutex, Condvar, RwLock, Once, and thread parking within std's platform-selection macros. ChangesVibix Threading and Futex Integration
Sequence DiagramsequenceDiagram
actor UserCode
participant std_thread
participant vibix_thread
participant vibix_abi
participant Kernel
UserCode->>std_thread: spawn(closure)
std_thread->>vibix_thread: Thread::new(stack_size, init)
vibix_thread->>vibix_thread: allocate mmap stack
vibix_thread->>vibix_thread: allocate ThreadData + store on stack
vibix_thread->>vibix_abi: clone(flags, stack, child_tid)
vibix_abi->>Kernel: SYS_CLONE
Kernel-->>vibix_abi: child_tid
vibix_abi-->>vibix_thread: parent returns Thread, child starts trampoline
vibix_thread->>vibix_thread: thread_trampoline() runs closure, cleanup
vibix_thread->>Kernel: SYS_EXIT (child only)
UserCode->>std_thread: join()
std_thread->>vibix_thread: Thread::join()
vibix_thread->>vibix_thread: loop: read child_tid atomic
vibix_thread->>vibix_abi: futex_wait on child_tid
vibix_abi->>Kernel: SYS_FUTEX (FUTEX_WAIT)
Kernel-->>vibix_abi: woken when child calls SYS_EXIT
vibix_thread->>vibix_thread: unmap stack, drop ThreadData
vibix_thread-->>UserCode: join() returns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes introduce multiple layers of interdependent functionality: low-level syscall wrappers, complex memory management with manual stack allocation and mmap, inline assembly for clone, futex-based synchronization primitives, and a trampoline function for thread initialization. While most changes are straightforward cfg routing, the vibix.rs thread implementation contains dense logic covering stack alignment, ThreadData lifecycle, raw pointer manipulation, and syscall error handling that requires careful review. Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
library/std/src/sys/thread/vibix.rs (1)
198-206: 💤 Low valueConsider using a cleaner type conversion.
The pointer cast from
AtomicU32toAtomic<u32>works but is fragile. If the internal futex API changes to use a different type, this could break silently.Consider whether the futex API could be extended to accept
&AtomicU32directly, or add a type alias/newtype to make this relationship explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/std/src/sys/thread/vibix.rs` around lines 198 - 206, The current code performs an unsafe pointer cast from AtomicU32 to crate::sync::atomic::Atomic<u32> when calling futex_wait, which is fragile; change the API or call site to avoid this raw cast by either (A) updating crate::sys::futex::futex_wait to accept &AtomicU32 directly, or (B) introduce a clear type alias/newtype that maps crate::sync::atomic::Atomic<u32> to AtomicU32 and use that alias at the call site, then pass &data.child_tid without the unsafe pointer trick; update the futex_wait signature and all callers (futex_wait) or add the alias/newtype and a safe conversion impl to replace the unsafe cast around data.child_tid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/std/src/sys/pal/vibix/futex.rs`:
- Around line 40-47: The current code turns oversized durations into None (via
try_into().ok()?), causing timeout_ptr to become 0 and thus turning a finite
timeout into an unbounded wait; change the conversion so Timespec is always
constructed and large durations are clamped instead of dropping to None: replace
the try_into().ok()? path in the timeout -> Timespec creation with a
saturating/clamping conversion (e.g., map dur.as_secs() to i64::MAX on overflow)
and set tv_nsec from dur.subsec_nanos(); keep the Some(Timespec { ... }) to
ensure timeout_ptr is non-zero for finite timeouts (identifiers: Timespec,
timeout, timeout_ptr).
---
Nitpick comments:
In `@library/std/src/sys/thread/vibix.rs`:
- Around line 198-206: The current code performs an unsafe pointer cast from
AtomicU32 to crate::sync::atomic::Atomic<u32> when calling futex_wait, which is
fragile; change the API or call site to avoid this raw cast by either (A)
updating crate::sys::futex::futex_wait to accept &AtomicU32 directly, or (B)
introduce a clear type alias/newtype that maps crate::sync::atomic::Atomic<u32>
to AtomicU32 and use that alias at the call site, then pass &data.child_tid
without the unsafe pointer trick; update the futex_wait signature and all
callers (futex_wait) or add the alias/newtype and a safe conversion impl to
replace the unsafe cast around data.child_tid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5696684-097e-488c-9f60-0711c2dafdd6
📒 Files selected for processing (12)
library/std/src/sys/pal/vibix/futex.rslibrary/std/src/sys/pal/vibix/mod.rslibrary/std/src/sys/sync/condvar/mod.rslibrary/std/src/sys/sync/mutex/mod.rslibrary/std/src/sys/sync/once/mod.rslibrary/std/src/sys/sync/rwlock/mod.rslibrary/std/src/sys/sync/thread_parking/mod.rslibrary/std/src/sys/thread/mod.rslibrary/std/src/sys/thread/vibix.rsuserspace/vibix_abi/src/futex.rsuserspace/vibix_abi/src/lib.rsuserspace/vibix_abi/src/thread.rs
…d fork
Completes Phase 3 of the std-on-vibix effort by enabling std::thread::spawn,
std::sync::Mutex, Condvar, RwLock, Once, and thread parking for vibix.
Changes:
- vibix_abi: add futex.rs (FUTEX_WAIT/WAKE wrappers) and thread.rs (clone,
sched_yield, nanosleep, gettid, set_tid_address, join_thread)
- std PAL: add sys/pal/vibix/futex.rs providing the Futex/SmallFutex types
and futex_wait/futex_wake/futex_wake_all that sys/sync/ primitives need
- std thread: add sys/thread/vibix.rs with Thread::new (clone-based spawn
with mmap'd stack), Thread::join (futex wait on CHILD_CLEARTID),
yield_now, sleep, available_parallelism, set_name (stub)
- std sync: wire target_os = "vibix" into the futex branches of mutex,
condvar, rwlock, once, and thread_parking cfg_select! dispatchers
The std fork builds cleanly with:
cargo +nightly -Z build-std=std,core,alloc,panic_abort \
-Z json-target-spec --target x86_64-unknown-vibix.json
Closes #857
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clamp oversized durations to i64::MAX instead of silently converting to an unbounded wait (the try_into().ok()? path dropped to None) - Use Atomic<u32> directly in ThreadData to avoid the fragile pointer cast between AtomicU32 and Atomic<u32> in join() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6bb19c8 to
ae9295b
Compare
Summary
sys/thread/vibix.rs:Thread::new()viaclonewith mmap'd stack,Thread::join()via futex wait onCHILD_CLEARTID,yield_now(),sleep(),available_parallelism()(returns 1),set_name()(stub)sys/pal/vibix/futex.rs) providingFutex/SmallFutextypes andfutex_wait/futex_wake/futex_wake_allfor the sync primitivestarget_os = "vibix"into the futex branches ofMutex,Condvar,RwLock,Once, andParkercfg_select! dispatchersvibix_abi::futexandvibix_abi::threadmodules with syscall wrappers forclone,futex,sched_yield,nanosleep,gettid,set_tid_addressThis completes Phase 3 and unblocks multi-threaded std programs on vibix.
Test plan
cargo xtask build-- kernel builds cleanly__CARGO_TESTS_ONLY_SRC_ROOT=library cargo +nightly build -Z build-std=std,core,alloc,panic_abort -Z json-target-spec --target x86_64-unknown-vibix.json(only unrelated profiler_builtins failure from missing LLVM source)Closes #857
🤖 Generated with Claude Code