Skip to content

fix(runtime): unify the three pthread stack-bounds extern declarations - #9776

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/main-green-9730
Closed

fix(runtime): unify the three pthread stack-bounds extern declarations#9776
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/main-green-9730

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The break

main cannot compile perry-runtime under -D warnings, so the warnings job on
https://github.com/PerryTS/perry/actions/runs/33926006467 (main at 12efed12220e) fails
with build failed, waiting for other jobs to finish:

error: `pthread_getattr_np` redeclared with a different signature
   --> crates/perry-runtime/src/gc/roots.rs:761:9
761 |         fn pthread_getattr_np(thread: usize, attr: *mut [u64; 8]) -> i32;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
    ::: crates/perry-runtime/src/error_stack_frames.rs:200:13
200 |             fn pthread_getattr_np(thread: usize, attr: *mut u8) -> i32;
    |             ------------------------------------------------------- `pthread_getattr_np` previously declared here
    = note: expected `unsafe extern "C" fn(usize, *mut u8) -> i32`
               found `unsafe extern "C" fn(usize, *mut [u64; 8]) -> i32`
    = note: `-D clashing-extern-declarations` implied by `-D warnings`
error: could not compile `perry-runtime` (lib) due to 3 previous errors

Same for pthread_attr_getstack and pthread_attr_destroy.

Root cause

perry-runtime declares these three libc functions in three separate extern "C"
blocks, in two different spellings:

site attr spelling cfg
gc/roots.rs:757 get_stack_bottom [u64; 8] target_os = "linux"
gc/roots/stack_maps.rs:1788 fp_chain::stack_top *mut u8 target_os = "linux" and target_arch = "aarch64"
error_stack_frames.rs:197 stack_top_uncached *mut u8 target_os = "linux" (no arch gate)

The *mut u8 spelling is not new — but the only site that used it was inside
mod fp_chain, which is gated to target_arch = "aarch64". On CI's linux-x86_64
host that module is cfg'd out, so the two spellings never met and the lint never fired.

Culprit: 1ebc65e87 — "feat(runtime): real function names in Error stacks — frame-pointer
walk + the existing name registry (#9486) (#9521)" (2026-09-02).
It added a third copy of
the block in error_stack_frames.rs gated only on target_os = "linux". With no arch gate
it collides with gc/roots.rs on x86_64, and the crate stopped compiling under -D warnings.

This is macOS-invisible: every declaration involved is behind cfg(target_os = "linux").

The fix

All three blocks now use the [u64; 8] spelling that gc::roots::get_stack_bottom — the
oldest and most-shipped of the three — has always used. This also clears the latent
aarch64-linux collision between fp_chain and gc/roots.rs, which would have broken that
target the moment anyone built it with -D warnings.

[u64; 8] is 64 bytes: at least the size of pthread_attr_t on every supported glibc/musl
target (56 bytes on both), and correctly aligned for it — which the [u8; 128] buffer it
replaces, with alignment 1, was not. No behavioural change otherwise; all three functions
compute the same stack-high address they did before.

Verification

  • Minimised standalone repro: the two spellings in one crate produce exactly the two
    redeclared with a different signature errors under -D warnings; unified, rustc
    exits 0.
  • RUSTFLAGS="-D warnings" cargo check -p perry-runtime --all-targets on a linux-x86_64
    host reproduces the 3 errors at 12efed12220e and passes with this commit.

Not fixed here

main is red for four further, independent reasons — none of them this compile error, and
none of them fixed by this PR. Detailed separately:

  • check — API-docs drift: Bun.connect/Bun.listen were added to the API manifest by
    868787448 ("feat(bun): add TCP socket facades") without re-running scripts/regen_api_docs.sh.
  • ext-link (red since 2026-09-04) — js_bun_tcp_listen calls perry_ffi::run_pending,
    and crates/perry-ext-http/src/test_async_shims.rs has no perry_ffi_run_pending stub.
    Same culprit commit 868787448.
  • cargo-testcommands::compile::build_cache::tests::codegen_env_vars_are_build_cache_inputs.
  • gc-stress matrix (4/4) / gap-suite (2) — output mismatches in
    test_gap_repsel_gc_stress, test_gap_perfhooks_3088_3008_3010_3011 and
    test_gap_prop_plan_cache_invalidation.

gc-stress and main-gate are aggregator jobs (14s / 3s) and go green once their
dependencies do.

https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Linux builds with warnings treated as errors.
    • Improved compatibility across supported Linux architectures by aligning system-level stack handling declarations.
    • Corrected buffer alignment and sizing used during thread stack inspection.

Ralph Küpper added 2 commits September 5, 2026 06:40
`perry-runtime` declared `pthread_getattr_np`, `pthread_attr_getstack` and
`pthread_attr_destroy` in three separate `extern "C"` blocks using two
different argument spellings. Declaring one symbol twice in a crate with
different signatures is `clashing_extern_declarations`, which `-D warnings`
denies, so the `warnings` job could not compile the crate at all:

    error: `pthread_getattr_np` redeclared with a different signature
      --> crates/perry-runtime/src/gc/roots.rs:761:9
      ::: crates/perry-runtime/src/error_stack_frames.rs:200:13
      = note: expected `unsafe extern "C" fn(usize, *mut u8) -> i32`
                 found `unsafe extern "C" fn(usize, *mut [u64; 8]) -> i32`
      = note: `-D clashing-extern-declarations` implied by `-D warnings`
    error: could not compile `perry-runtime` (lib) due to 3 previous errors

`gc::roots::get_stack_bottom` has spelled the attr buffer `[u64; 8]` since
long before the break. The competing `*mut u8` spelling also predates it, in
`gc::roots::stack_maps::fp_chain`, but that module is gated to
`target_arch = "aarch64"`, so on CI's linux-x86_64 host the two never met.
1ebc65e (PerryTS#9486/PerryTS#9521) added a third copy in `error_stack_frames.rs` gated
only on `target_os = "linux"`; with no arch gate it collides with
`gc/roots.rs` on x86_64, and the crate stopped compiling under `-D warnings`.

All three blocks now use the `[u64; 8]` spelling, which additionally clears
the latent aarch64-linux collision between `fp_chain` and `gc/roots.rs`.
`[u64; 8]` is 64 bytes — at least the size of `pthread_attr_t` on every
supported glibc/musl target, and correctly aligned for it, which the
`[u8; 128]` buffer it replaces was not.

Unbreaks the `warnings` job on
https://github.com/PerryTS/perry/actions/runs/33926006467

Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 1e259a73-684d-44d8-96b6-b67ef1a4f609

📥 Commits

Reviewing files that changed from the base of the PR and between 12efed1 and 7ce6e9f.

📒 Files selected for processing (3)
  • changelog.d/9776-pthread-extern-signature-clash.md
  • crates/perry-runtime/src/error_stack_frames.rs
  • crates/perry-runtime/src/gc/roots/stack_maps.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The Linux pthread declarations now use a consistent [u64; 8] attribute representation and typed stack-address pointers. The changes remove conflicting extern signatures, provide correct alignment, and document the fix.

Changes

Linux pthread stack binding updates

Layer / File(s) Summary
Align pthread stack bindings
crates/perry-runtime/src/error_stack_frames.rs, crates/perry-runtime/src/gc/roots/stack_maps.rs, changelog.d/9776-pthread-extern-signature-clash.md
The stack discovery paths now use matching [u64; 8] pthread attribute buffers and typed u8 stack-address pointers. Existing cleanup and error handling remain unchanged. The changelog records the clashing_extern_declarations fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 7ce6e

This change unifies Linux pthread stack-discovery bindings, restoring warning-free compilation and removing inconsistent extern declarations. No merge-blocking production risk is identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: unifying the three pthread stack-bounds extern declarations.
Description check ✅ Passed The description is detailed and on-topic. It explains the failure, root cause, fix, verification, and unrelated CI failures. It does not use the template headings and omits the Related issue and Check…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Conflicts with merge train #9798, which just landed 19 PRs (including #9750's rework of gc_runtime_root_holders.py and a regex.rs split under the 2000-line cap). Could you rebase onto current main? I'd rather you resolve it than have me hand-merge — several of these touch GC root scanning or regex internals where the two changes are independent rewrites of the same code, and that's exactly where a mechanical merge goes quietly wrong. Everything that picked clean is in the next train; I'll pick these up as soon as they rebase.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Superseded — closing. This PR unifies the three pthread stack-bounds extern declarations, but main already does that via 3bfbe40e9 (#9752, landed in train #9798), which introduced the shared native_stack::stack_top() helper. Only two files declare pthread_getattr_np now (native_stack.rs itself and gc/roots/stack_bottom.rs), down from three, and callers go through the helper rather than repeating the extern block per file — which is the stronger form of what this PR proposed. Nothing here is lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant