Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
153 changes: 77 additions & 76 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ codegen-units = 16
codegen-units = 16

[workspace.package]
version = "0.5.1290"
version = "0.5.1291"
edition = "2021"
license = "MIT"
repository = "https://github.com/PerryTS/perry"
Expand Down
28 changes: 28 additions & 0 deletions changelog.d/7509-integration-suites-native-roots.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
### Fixed

**`perry-codegen` integration suites can declare which root lowering they assert (#7493).**

#7370 made native roots (RS4GC statepoints) the default lowering, and `NativeRootsPin` was added so a test asserting on shadow-stack IR could say so. The in-crate unit tests were repaired with it; the five integration suites that assert the same mechanics were not, because `NativeRootsPin` is `#[cfg(test)]` and `tests/*.rs` link the crate as an ordinary external consumer — the item does not exist for them. Those suites run nightly/at-tag only, so nothing turned red at merge time and `shadow_slot_hygiene` sat at 0/12 on `main`.

The pin is now reachable as `perry_codegen::testing::NativeRootsPin`, behind a `testing` cargo feature that only `perry-codegen`'s own `[dev-dependencies]` entry on itself enables. It is a feature rather than a `#[doc(hidden)] pub` because with the feature off the pin, its thread-local and the branch it adds to `rs4gc_enabled()` are `#[cfg]`-ed out of the artifact — absent, not private — and cargo resolves dev-dependency features only for test/bench targets, so no production profile can reach it (`cargo tree -e features,no-dev` shows `default` + `llvm-inprocess` only). `NativeRootsPin::native()` joins `shadow()`: a pin also outranks `PERRY_RS4GC`, which is what keeps a pinned assertion meaning the same thing during a `PERRY_RS4GC=0` sweep.

Pins are per test, not per file — the files disagree internally:

| suite | shadow | native | unpinned | before → after |
|---|---|---|---|---|
| `shadow_slot_hygiene` | 12 | – | – | 0/12 → 11/12 |
| `scalar_replaced_slot_roots` | 11 | – | – | 2/11 → 5/11 |
| `temp_root_operand_temporaries` | 2 | – | 17 | 12/19 → 13/19 |
| `temp_root_argument_temporaries` | – | – | 7 | 3/7 → 3/7 |
| `native_proof_regressions` | 2 | 15 | 238 | 249/253 → 253/255 |
| `native_proof_buffer_views` | – | 1 | 31 | 28/30 → 30/32 |

Three tests were pinned although they were **passing**: `numeric_only_scalar_replaced_{object,array}_emits_no_rooting` and `a_collection_free_construction_emits_no_this_slot_root` assert `bind_calls(&ir) == 0` / `!contains("@js_shadow_slot_bind")`, which under the native default is true of every program. They were green without their subject running. Two of the three now fail for a real reason (#7504) — a red test that measures something beats a green one that does not.

The fifteen `invalidation` pins are `native()` for a different reason: `assert_buffer_store_uses_dynamic_fallback` proves the absence of a native buffer GEP with a module-wide `!ir.contains("getelementptr inbounds i8")`, and the shadow lowering's inline slot addressing emits that instruction for unrelated reasons, so `PERRY_RS4GC=0` made them report a stale proof that was never there (#7505).

**A poisoned `ARTIFACT_ENV_LOCK` turned 4 failures into 55.** `native_proof_regressions` reported 55 failures at default parallelism and 4 under `--test-threads=1`; 51 of the 55 were `PoisonError` — #7490's shape again. `PERRY_NATIVE_REPS*` are process-global and the restore was hand-written after the compile, so a panic inside `compile_module` left them installed and every later unlocked compile wrote artifact JSON into a directory another test was reading; the torn read panicked inside the lock. Both suites' copy-pasted harnesses are replaced by one `tests/native_proof_support/mod.rs` with a poison-tolerant accessor, an RAII env guard and an artifact reader that treats a foreign or half-written neighbour as noise. Two sabotage tests plant each failure shape and fail against the pre-fix code. Default-parallelism result: 198/253 → 253/255.

**Tripwire, in the required tier.** `codegen::testing_feature_gate_tests::host_target_lowering_default_is_native_roots` lives in `src/`, so it runs in `cargo-test`: a future flip of the lowering default fails there, in the PR that makes it, naming the suites that then need re-pinning. It asserts its subject is live (both pins must give different answers; the `arm64_32-apple-watchos` arm must give the opposite default), so a constant-folded `rs4gc_enabled()` fails it rather than passing it. A sibling gate scans every workspace manifest and fails if a non-dev dependency edge ever enables the `testing` feature.

Fifteen failures remain across these suites, none of them #7370's and none hidden: #7494 (three, pre-existing), #7503 (ten — the temp-root suites assert the pre-#7487 FFI spelling, and the eight that still pass are vacuous), #7504 (six plus `flat_const_row_aliases`), #7506 (one). No test was deleted, skipped or weakened. The per-PR source→suite mapping for these six suites is designed in #7507 and deliberately **not** landed here: they are not green, and a non-required job that is red on most PRs can never be promoted. The coverage question the pins raise — nine root-lowering mechanics with no assertion against the lowering that actually ships — is #7502.
21 changes: 21 additions & 0 deletions crates/perry-codegen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ workspace = true
default = ["llvm-inprocess"]
llvm-inprocess = ["dep:inkwell", "dep:llvm-sys"]

# Test-support only (#7493): exposes `perry_codegen::testing`, whose
# `NativeRootsPin` lets a test declare WHICH root lowering it asserts on. It is
# a feature rather than a `#[doc(hidden)] pub` so that with the feature off the
# pin, its thread-local and the branch it adds to `rs4gc_enabled()` are not in
# the artifact at all — see `src/testing.rs`.
#
# The ONLY edge that enables it is the `[dev-dependencies]` entry below, which
# cargo builds for test/bench targets only. `codegen::helpers::
# testing_feature_gate_tests` fails the per-PR `cargo-test` job if any
# non-dev manifest section ever turns it on.
testing = []

[dependencies]
perry-hir.workspace = true
perry-dispatch.workspace = true
Expand All @@ -41,3 +53,12 @@ serde_json.workspace = true

inkwell = { version = "0.9.0", features = ["llvm22-1"], optional = true }
llvm-sys = { version = "221", optional = true }

# Self dev-dependency (#7493). This is the whole mechanism by which the
# integration suites under `tests/` — which link this crate as an ordinary
# external consumer — can see `perry_codegen::testing`. Cargo supports
# dev-dependency cycles and resolves dev-dependency features ONLY when a
# test/bench target is being built, so `cargo build`/`--release`/`--profile
# dist` still compile the library with `testing` off.
[dev-dependencies]
perry-codegen = { path = ".", features = ["testing"] }
65 changes: 44 additions & 21 deletions crates/perry-codegen/src/codegen/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ pub(crate) fn precise_root_analysis_enabled() -> bool {
/// explicit bridge's hand emission and its conservative CFG-union liveness.
/// Requires an `opt` binary (`PERRY_LLVM_OPT`, Homebrew LLVM, or PATH).
pub(crate) fn rs4gc_enabled() -> bool {
#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
if let Some(pinned) = NATIVE_ROOTS_OVERRIDE.with(|c| c.get()) {
return pinned;
}
Expand Down Expand Up @@ -143,38 +143,61 @@ thread_local! {
static NATIVE_ROOTS_TARGET_OK: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
}

/// Test-only RAII pin for the lowering under test.
// The pin's backing cell. Separate from `NATIVE_ROOTS_TARGET_OK` on purpose:
// `compile_module` calls `set_native_roots_for_target` per module, so a pin
// that wrote the target cell would be overwritten the moment the test invoked
// codegen. This is consulted FIRST and the per-module decision cannot clear it.
#[cfg(any(test, feature = "testing"))]
thread_local! {
static NATIVE_ROOTS_OVERRIDE: std::cell::Cell<Option<bool>> =
const { std::cell::Cell::new(None) };
}

/// Test-support RAII pin for the root lowering under test.
///
/// Now that native roots are the default, a test that asserts on shadow-stack
/// IR has to SAY so — it used to get that lowering by accident, because there
/// was only one default. Eight tests broke on exactly this when the default
/// flipped, and every one of them was correct about what it asserted.
/// was only one default. Eight in-crate tests broke on exactly this when the
/// default flipped, and every one of them was correct about what it asserted;
/// five integration suites broke the same way and stayed red for weeks, because
/// this type was `#[cfg(test)]` and they could not reach it (#7493).
///
/// Thread-local and restoring, so one test pinning a lowering cannot change
/// another's — the same discipline `arena::quarantine`'s `ProtectionModeGuard`
/// already uses for the from-space instrument.
#[cfg(test)]
thread_local! {
/// Separate from `NATIVE_ROOTS_TARGET_OK` on purpose: `compile_module`
/// calls `set_native_roots_for_target` per module, so a pin that wrote the
/// target cell would be overwritten the moment the test invoked codegen.
/// This is consulted FIRST and the per-module decision cannot clear it.
static NATIVE_ROOTS_OVERRIDE: std::cell::Cell<Option<bool>> =
const { std::cell::Cell::new(None) };
}

#[cfg(test)]
pub(crate) struct NativeRootsPin(Option<bool>);
/// already uses for the from-space instrument. Safe under `cargo test`'s
/// default parallelism.
///
/// Reachable from `tests/*.rs` as `perry_codegen::testing::NativeRootsPin`; see
/// [`crate::testing`] for why that is behind a cargo feature rather than an
/// unconditional `pub`.
#[cfg(any(test, feature = "testing"))]
pub struct NativeRootsPin(Option<bool>);

#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
impl NativeRootsPin {
/// Pin this thread to the shadow-stack lowering for the guard's lifetime.
pub(crate) fn shadow() -> Self {
/// Pin this thread to the **shadow-stack** lowering for the guard's
/// lifetime — Perry's heap-backed shadow frame, `js_shadow_frame_enter` +
/// per-slot binds.
pub fn shadow() -> Self {
NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(false))))
}

/// Pin this thread to the **native-roots** (RS4GC statepoint) lowering:
/// `ptr addrspace(1)` root allocas, `gc "statepoint-example"`, relocations
/// inserted by LLVM.
///
/// This is today's default on every target the runtime can walk, so a test
/// that wants it does not strictly *need* the pin — but a pin is not
/// redundant: it also overrides `PERRY_RS4GC` from the environment, so the
/// assertion means the same thing during a `PERRY_RS4GC=0` bisection run as
/// it does in CI. Without it, a whole-suite sweep under the process-global
/// env knob silently retargets every unpinned test at the other lowering.
pub fn native() -> Self {
NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(true))))
}
}

#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
impl Drop for NativeRootsPin {
fn drop(&mut self) {
NATIVE_ROOTS_OVERRIDE.with(|c| c.set(self.0));
Comment on lines +173 to 203

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 3 'struct NativeRootsPin|impl (Send|Sync) for NativeRootsPin|NATIVE_ROOTS_OVERRIDE|thread::spawn' \
  crates/perry-codegen/src crates/perry-codegen/tests

Repository: PerryTS/perry

Length of output: 5148


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
import pathlib, re

p = pathlib.Path("crates/perry-codegen/src/codegen/helpers.rs")
text = p.read_text()

struct = re.search(r'#[^\n]*\npub struct NativeRootsPin\(([^)]+)\)', text, re.M)
drop  = re.search(r'impl Drop for NativeRootsPin\s*\{[^}]+fn drop\(&mut self\)\s*\{([^}]+)\}\s*\}', text, re.S)

print("struct:", struct.group(0) if struct else "NOT FOUND")
fields = struct.group(1) if struct else None
print("field_type:", fields)
print("drop_body:", drop.group(1).strip() if drop else "NOT FOUND")
print("mentions_NATIVE_ROOTS_OVERRIDE_in_drop:", "NATIVE_ROOTS_OVERRIDE" in (drop.group(1).strip() if drop else ""))
print("has_phantom_Rc :", "PhantomData<&'static (); PhantomData<std" in text or "PhantomData<std::rc::Rc" in text)

# Deterministic Rust marker model: a struct is Send/Sync iff all fields are Send/Sync, etc.
python3 - <<'PY'
fields = ("Option<bool>",)
sentinel = ("Rc<()>",)
def has_send(t):
    return t in ("Option<bool>",)
def has_sync(t):
    return t in ("Option<bool>",)
print("NativeRootsPin currently Send/Sync?", all(has_send(fields[0])), all(has_sync(fields[0])))
print("NativeRootsPin with PhantomData<Rc<()>> Send/Sync?", all(f in sentinel or f in ("Option<bool>",) for f in fields))
PY
PY

Repository: PerryTS/perry

Length of output: 242


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
import pathlib, re

p = pathlib.Path("crates/perry-codegen/src/codegen/helpers.rs")
text = p.read_text()

struct = re.search(r'#[^\n]*\npub struct NativeRootsPin\(([^)]+)\)', text, re.M)
drop = re.search(r'impl Drop for NativeRootsPin\s*\{[^}]+fn drop\(&mut self\)\s*\{([^}]+)\}\s*\}', text, re.S)

print("struct:", struct.group(0) if struct else "NOT FOUND")
fields = struct.group(1) if struct else None
print("field_type:", fields)
print("drop_body:", drop.group(1).strip() if drop else "NOT FOUND")
if drop:
    print("has_NATIVE_ROOTS_OVERRIDE_in_drop:", "NATIVE_ROOTS_OVERRIDE" in drop.group(1).strip())
print("has_phantom_Rc :", "PhantomData<std::rc::Rc" in text)

print("NativeRootsPin currently Send/Sync?", (fields and fields.strip().startswith("Option<bool>")))
PY

Repository: PerryTS/perry

Length of output: 425


Make NativeRootsPin thread-affine.

NativeRootsPin(Option<bool>) is Send, so a caller can move the guard to another thread. Drop restores NATIVE_ROOTS_OVERRIDE on that thread instead, while the original thread remains pinned and can use the wrong lowering.

Proposed fix
-pub struct NativeRootsPin(Option<bool>);
+pub struct NativeRootsPin(
+    Option<bool>,
+    std::marker::PhantomData<std::rc::Rc<()>>,
+);
@@
-        NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(false))))
+        NativeRootsPin(
+            NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(false))),
+            std::marker::PhantomData,
+        )
@@
-        NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(true))))
+        NativeRootsPin(
+            NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(true))),
+            std::marker::PhantomData,
+        )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[cfg(any(test, feature = "testing"))]
pub struct NativeRootsPin(Option<bool>);
#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
impl NativeRootsPin {
/// Pin this thread to the shadow-stack lowering for the guard's lifetime.
pub(crate) fn shadow() -> Self {
/// Pin this thread to the **shadow-stack** lowering for the guard's
/// lifetime — Perry's heap-backed shadow frame, `js_shadow_frame_enter` +
/// per-slot binds.
pub fn shadow() -> Self {
NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(false))))
}
/// Pin this thread to the **native-roots** (RS4GC statepoint) lowering:
/// `ptr addrspace(1)` root allocas, `gc "statepoint-example"`, relocations
/// inserted by LLVM.
///
/// This is today's default on every target the runtime can walk, so a test
/// that wants it does not strictly *need* the pin — but a pin is not
/// redundant: it also overrides `PERRY_RS4GC` from the environment, so the
/// assertion means the same thing during a `PERRY_RS4GC=0` bisection run as
/// it does in CI. Without it, a whole-suite sweep under the process-global
/// env knob silently retargets every unpinned test at the other lowering.
pub fn native() -> Self {
NativeRootsPin(NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(true))))
}
}
#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
impl Drop for NativeRootsPin {
fn drop(&mut self) {
NATIVE_ROOTS_OVERRIDE.with(|c| c.set(self.0));
#[cfg(any(test, feature = "testing"))]
pub struct NativeRootsPin(
Option<bool>,
std::marker::PhantomData<std::rc::Rc<()>>,
);
#[cfg(any(test, feature = "testing"))]
impl NativeRootsPin {
/// Pin this thread to the **shadow-stack** lowering for the guard's
/// lifetime — Perry's heap-backed shadow frame, `js_shadow_frame_enter` +
/// per-slot binds.
pub fn shadow() -> Self {
NativeRootsPin(
NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(false))),
std::marker::PhantomData,
)
}
/// Pin this thread to the **native-roots** (RS4GC statepoint) lowering:
/// `ptr addrspace(1)` root allocas, `gc "statepoint-example"`, relocations
/// inserted by LLVM.
///
/// This is today's default on every target the runtime can walk, so a test
/// that wants it does not strictly *need* the pin — but a pin is not
/// redundant: it also overrides `PERRY_RS4GC` from the environment, so the
/// assertion means the same thing during a `PERRY_RS4GC=0` bisection run as
/// it does in CI. Without it, a whole-suite sweep under the process-global
/// env knob silently retargets every unpinned test at the other lowering.
pub fn native() -> Self {
NativeRootsPin(
NATIVE_ROOTS_OVERRIDE.with(|c| c.replace(Some(true))),
std::marker::PhantomData,
)
}
}
#[cfg(any(test, feature = "testing"))]
impl Drop for NativeRootsPin {
fn drop(&mut self) {
NATIVE_ROOTS_OVERRIDE.with(|c| c.set(self.0));
}
}
🤖 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 `@crates/perry-codegen/src/codegen/helpers.rs` around lines 173 - 203, Make
NativeRootsPin non-Send so it cannot be moved across threads and dropped on a
different thread. Update the NativeRootsPin definition using an existing
thread-affinity marker pattern, while preserving its shadow(), native(), and
Drop behavior on the creating thread.

Expand Down
2 changes: 2 additions & 0 deletions crates/perry-codegen/src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ mod number_exactness_tests;
mod opts;
mod spec_abi;
mod string_pool;
#[cfg(test)]
mod testing_feature_gate_tests;
mod typed_abi;
mod typed_abi_opt_report;

Expand Down
Loading
Loading