diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0bb5acf0..f93d144c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,6 +77,8 @@ jobs: run: bash testing/check-action-pins.sh - name: Check every source comment resolves in-repo run: bash testing/check-comment-refs.sh + - name: Check no non-test code uses std 64-bit atomics + run: python3 testing/check-portable-atomics.py # Hermetic: synthetic ping functions, no containers, ~45s. Lives beside # the other two so both runners gate on it identically — putting it in # only one would create exactly the drift check-ci-parity.sh exists to diff --git a/src/instr/capture.rs b/src/instr/capture.rs index 71b27e1e..9d231187 100644 --- a/src/instr/capture.rs +++ b/src/instr/capture.rs @@ -10,11 +10,12 @@ //! connection is served by its own spawned task, so two simultaneous `on` //! requests are genuinely concurrent and must not both create a writer. +use portable_atomic::AtomicU64; use std::fs::File; use std::io::Write; use std::path::{Component, Path, PathBuf}; use std::sync::Mutex; -use std::sync::atomic::{AtomicBool, AtomicU8, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use super::recorder; diff --git a/src/instr/recorder.rs b/src/instr/recorder.rs index 491602ca..93bd51b6 100644 --- a/src/instr/recorder.rs +++ b/src/instr/recorder.rs @@ -9,8 +9,8 @@ //! `swap(0)`, so there are no "previous value" arrays to carry and the counters //! are per-interval by construction. +use portable_atomic::{AtomicU64, Ordering::Relaxed}; use std::sync::LazyLock; -use std::sync::atomic::{AtomicU64, Ordering::Relaxed}; use std::time::{Duration, Instant}; /// Measurement domain. Structural only: one variant today. diff --git a/src/native/mod.rs b/src/native/mod.rs index b7c65f25..57a772f4 100644 --- a/src/native/mod.rs +++ b/src/native/mod.rs @@ -81,11 +81,12 @@ mod unix_impl { use crate::config::NativeApiConfig; use crate::control::protocol::{Request, Response}; use crate::identity::{NodeAddr, decode_npub, encode_npub}; + use portable_atomic::AtomicU64; use secp256k1::XOnlyPublicKey; use std::collections::HashMap; use std::os::fd::{AsFd, OwnedFd}; use std::path::PathBuf; - use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use tokio::io::BufReader; use tokio::net::{UnixListener, UnixStream}; diff --git a/src/node/dataplane/connected_udp.rs b/src/node/dataplane/connected_udp.rs index b22419f8..090dd315 100644 --- a/src/node/dataplane/connected_udp.rs +++ b/src/node/dataplane/connected_udp.rs @@ -34,7 +34,7 @@ use crate::node::Node; #[cfg(any(target_os = "linux", target_os = "macos"))] use crate::transport::TransportHandle; #[cfg(any(target_os = "linux", target_os = "macos"))] -use std::sync::atomic::{AtomicU64, Ordering::Relaxed}; +use portable_atomic::{AtomicU64, Ordering::Relaxed}; #[cfg(any(target_os = "linux", target_os = "macos"))] use tracing::{debug, warn}; diff --git a/src/node/decrypt_worker.rs b/src/node/decrypt_worker.rs index 9566169a..a5d3e7aa 100644 --- a/src/node/decrypt_worker.rs +++ b/src/node/decrypt_worker.rs @@ -39,10 +39,10 @@ use crate::NodeAddr; use crate::transport::{TransportAddr, TransportId}; use crossbeam_channel::{Receiver, Sender, TrySendError, bounded}; +use portable_atomic::{AtomicU64, Ordering}; use ring::aead::{Aad, LessSafeKey, Nonce}; use std::collections::HashMap; use std::sync::Arc; -use std::sync::atomic::{AtomicU64, Ordering}; use tokio::sync::mpsc::UnboundedSender; use tracing::{debug, trace, warn}; diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index e29dde73..1c94a268 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -513,8 +513,7 @@ impl EncryptWorkerPool { match self.senders[idx].try_push(job) { Ok(()) => {} Err(MacWorkerTryPushError::Full(job)) => { - static FULL_COUNT: std::sync::atomic::AtomicU64 = - std::sync::atomic::AtomicU64::new(0); + static FULL_COUNT: portable_atomic::AtomicU64 = portable_atomic::AtomicU64::new(0); let n = FULL_COUNT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); if n < 8 || n.is_multiple_of(10000) { warn!( @@ -538,8 +537,7 @@ impl EncryptWorkerPool { match self.senders[idx].try_send(job) { Ok(()) => {} Err(TrySendError::Full(job)) => { - static FULL_COUNT: std::sync::atomic::AtomicU64 = - std::sync::atomic::AtomicU64::new(0); + static FULL_COUNT: portable_atomic::AtomicU64 = portable_atomic::AtomicU64::new(0); let n = FULL_COUNT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); if n < 8 || n.is_multiple_of(10000) { warn!( @@ -571,7 +569,7 @@ struct MacSendFlowKey { #[derive(Default)] struct MacSequencedSendFlows { flows: Mutex>>, - last_prune_ms: std::sync::atomic::AtomicU64, + last_prune_ms: portable_atomic::AtomicU64, } #[cfg(target_os = "macos")] @@ -719,8 +717,8 @@ struct MacSequencedSendFlow { socket: AsyncUdpSocket, connected_socket: Option>, dest_addr: SocketAddr, - next_seq: std::sync::atomic::AtomicU64, - last_used_ms: std::sync::atomic::AtomicU64, + next_seq: portable_atomic::AtomicU64, + last_used_ms: portable_atomic::AtomicU64, state: Mutex, ready_cv: Condvar, space_cv: Condvar, @@ -763,8 +761,8 @@ impl MacSequencedSendFlow { socket, connected_socket, dest_addr, - next_seq: std::sync::atomic::AtomicU64::new(0), - last_used_ms: std::sync::atomic::AtomicU64::new(now_ms), + next_seq: portable_atomic::AtomicU64::new(0), + last_used_ms: portable_atomic::AtomicU64::new(now_ms), state: Mutex::new(MacSendFlowState::default()), ready_cv: Condvar::new(), space_cv: Condvar::new(), @@ -1386,8 +1384,8 @@ impl SendBackpressurePacer { return false; } - static SEND_BACKPRESSURE_COUNT: std::sync::atomic::AtomicU64 = - std::sync::atomic::AtomicU64::new(0); + static SEND_BACKPRESSURE_COUNT: portable_atomic::AtomicU64 = + portable_atomic::AtomicU64::new(0); let n = SEND_BACKPRESSURE_COUNT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); if n < 8 || n.is_multiple_of(100_000) { warn!( @@ -1493,8 +1491,8 @@ fn default_send_backpressure_drop_after() -> u32 { #[cfg(all(unix, not(target_os = "linux")))] fn record_udp_send_backpressure_drop(err: &std::io::Error) { - static SEND_BACKPRESSURE_DROP_COUNT: std::sync::atomic::AtomicU64 = - std::sync::atomic::AtomicU64::new(0); + static SEND_BACKPRESSURE_DROP_COUNT: portable_atomic::AtomicU64 = + portable_atomic::AtomicU64::new(0); let n = SEND_BACKPRESSURE_DROP_COUNT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); if n < 8 || n.is_multiple_of(100_000) { warn!( diff --git a/src/node/metrics.rs b/src/node/metrics.rs index b5727d04..77f9ca6d 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -11,7 +11,7 @@ //! The remaining families (session, handshake, mmp, transport) stay on //! `NodeStats`. -use std::sync::atomic::{AtomicU64, Ordering}; +use portable_atomic::{AtomicU64, Ordering}; use crate::cache::HintOutcome; use crate::node::reject::{BloomReject, DiscoveryReject, ForwardingReject, TreeReject}; diff --git a/src/nostr/runtime.rs b/src/nostr/runtime.rs index 557441b0..7d1ae7b6 100644 --- a/src/nostr/runtime.rs +++ b/src/nostr/runtime.rs @@ -1,7 +1,8 @@ +use portable_atomic::AtomicU64; use std::collections::{HashMap, HashSet}; use std::net::SocketAddr; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::{Duration, Instant}; use nostr::nips::nip17; diff --git a/src/perf_profile.rs b/src/perf_profile.rs index 2745823a..66442954 100644 --- a/src/perf_profile.rs +++ b/src/perf_profile.rs @@ -36,8 +36,8 @@ //! * `FMP_WORKER_QUEUE_WAIT` — rx_loop FMP job dispatch → worker //! * `ENDPOINT_EVENT_WAIT` — rx_loop endpoint delivery → endpoint recv +use portable_atomic::{AtomicU64, Ordering::Relaxed}; use std::sync::OnceLock; -use std::sync::atomic::{AtomicU64, Ordering::Relaxed}; use std::time::Instant; /// Number of measurement buckets. Indices match `Stage`. diff --git a/testing/check-portable-atomics.py b/testing/check-portable-atomics.py new file mode 100755 index 00000000..d9443754 --- /dev/null +++ b/testing/check-portable-atomics.py @@ -0,0 +1,206 @@ +#!/usr/bin/env python3 +"""Fail when non-test code under src/ uses the std 64-bit atomics. + +`std::sync::atomic::AtomicU64` and `AtomicI64` exist only on targets with +64-bit atomics. The 32-bit MIPS targets the OpenWrt packages are meant to +cover (mips-unknown-linux-musl, mipsel-unknown-linux-musl) have none, so one +such use anywhere in the crate stops the whole crate building there. Code +that needs a 64-bit atomic uses `portable_atomic::AtomicU64` instead, which is +the std type where the target has one and a lock-based fallback where it does +not. No CI leg builds for MIPS yet, so without this check the first anyone +would hear of a new std use is a failed build on a router target. + +What is flagged, read from the files committed at HEAD: + + * a `use` tree rooted at `std` or `core` that reaches + `sync::atomic::AtomicU64` or `sync::atomic::AtomicI64`, in any form: + a plain path, a grouped `{...}` list over one or several lines, a nested + group such as `std::sync::{Arc, atomic::{AtomicU64, Ordering}}`, or a + rename with `as`; + * a glob import of `std::sync::atomic::*` or `core::sync::atomic::*`, + because it brings both types in unnamed; + * a path-qualified use such as `std::sync::atomic::AtomicU64::new(0)`, and + `atomic::AtomicU64` after `use std::sync::atomic;`, anywhere in the text; + * `sa::AtomicU64` where the file renames the module with + `use std::sync::atomic as sa;` or `use std::sync::{atomic::{self as sa}}`. + +Scope is src/, minus files under a `tests/` directory and files named +`tests.rs` or `*_tests.rs`, which are only ever built for the host. A +`#[cfg(test)]` module inside an ordinary file is NOT exempt: telling it apart +needs a Rust parser, and holding test code in those files to the same rule +costs nothing today (no such module uses the std types). + +One file is exempt by name, with its reason: src/transport/ble/io_android.rs +is compiled only for Android, and every Android target Rust supports has +64-bit atomics. + +Known gaps, recorded rather than discovered: a std 64-bit atomic reached +through a re-export from another crate, or through a type alias defined +outside src/, is not seen; nor is one written with a macro that assembles the +path from pieces. + +Exit codes: + 0 - no non-test file under src/ uses a std 64-bit atomic + 1 - at least one does; every hit is printed + 2 - the check could not look (not a git work tree, git failed, or src/ + matched no Rust files); never a pass +""" + +from __future__ import annotations + +import re +import subprocess +import sys + +EXEMPT = { + "src/transport/ble/io_android.rs": "Android targets all have 64-bit atomics", +} + +WIDE = ("AtomicU64", "AtomicI64") + +USE_RE = re.compile(r"\buse\s+([^;]+);", re.S) +# Any `atomic::AtomicU64` whose `atomic` is not the tail of a longer name, so +# `portable_atomic::AtomicU64` is not matched and every std spelling is. +PATH_RE = re.compile(r"(? str: + """Run a git command and return its stdout, exiting 2 if it fails.""" + proc = subprocess.run(["git", *args], capture_output=True, text=True) + if proc.returncode != 0: + print(f"check-portable-atomics: git {' '.join(args)} failed: {proc.stderr.strip()}", + file=sys.stderr) + sys.exit(2) + return proc.stdout + + +def is_test_path(path: str) -> bool: + """True for files only ever built as part of the test harness.""" + parts = path.split("/") + name = parts[-1] + return "tests" in parts[:-1] or name == "tests.rs" or name.endswith("_tests.rs") + + +def split_top(text: str) -> list[str]: + """Split a use-tree list on the commas that are not inside braces.""" + items, depth, cur = [], 0, [] + for ch in text: + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if ch == "," and depth == 0: + items.append("".join(cur)) + cur = [] + else: + cur.append(ch) + items.append("".join(cur)) + return [i.strip() for i in items if i.strip()] + + +def flatten(tree: str, prefix: tuple[str, ...] = ()) -> list[tuple[str, ...]]: + """Expand a use tree into the full paths it imports. + + A rename, written `name@alias` by the caller, stays on the last segment. + """ + tree = re.sub(r"\s+", "", tree) + brace = tree.find("{") + if brace == -1: + segs = tuple(s for s in tree.split("::") if s) + return [prefix + segs] + head = tuple(s for s in tree[:brace].split("::") if s) + inner = tree[brace + 1:tree.rfind("}")] + out = [] + for item in split_top(inner): + if item == "self" or item.startswith("self@"): + alias = item[len("self"):] + out.append(prefix + head[:-1] + (head[-1] + alias,)) + else: + out.extend(flatten(item, prefix + head)) + return out + + +def use_hits(text: str) -> list[tuple[int, str]]: + """Return (line, imported path) for each std/core wide-atomic import.""" + hits = [] + for m in USE_RE.finditer(text): + body = re.sub(r"\s+as\s+(\w+)", r"@\1", m.group(1)) + line = text.count("\n", 0, m.start()) + 1 + for path in flatten(body): + if not path: + continue + last, _, alias = path[-1].partition("@") + path = path[:-1] + (last,) + if path[0] not in ("std", "core") or path[1:3] != ("sync", "atomic"): + continue + if len(path) == 3 and alias: + hits.extend(alias_hits(text, alias, "::".join(path))) + elif len(path) >= 4 and (path[3] in WIDE or path[3] == "*"): + hits.append((line, "::".join(path[:4]))) + return hits + + +def alias_hits(text: str, alias: str, module: str) -> list[tuple[int, str]]: + """Return (line, use) for each wide atomic named through a module alias.""" + hits = [] + pat = re.compile(r"(? list[tuple[int, str]]: + """Return (line, matched text) for each path-qualified wide atomic.""" + hits = [] + for m in PATH_RE.finditer(text): + line = text.count("\n", 0, m.start()) + 1 + hits.append((line, re.sub(r"\s+", "", m.group(0)))) + return hits + + +def main() -> int: + """Scan the committed src/ tree and report every std wide-atomic use.""" + root = git("rev-parse", "--show-toplevel").strip() + if not root: + print("check-portable-atomics: empty work-tree root", file=sys.stderr) + return 2 + files = [ + f for f in git("-C", root, "ls-tree", "-r", "--name-only", "HEAD", "--", "src/").splitlines() + if f.endswith(".rs") + ] + if not files: + print("check-portable-atomics: src/ matched no Rust files at HEAD", file=sys.stderr) + return 2 + + scanned = 0 + findings = [] + for path in files: + if is_test_path(path) or path in EXEMPT: + continue + text = git("-C", root, "show", f"HEAD:{path}") + scanned += 1 + seen = set() + for line, what in use_hits(text) + path_hits(text): + if (line, what) not in seen: + seen.add((line, what)) + findings.append(f"{path}:{line}: {what}") + + if scanned == 0: + print("check-portable-atomics: every file under src/ was excluded", file=sys.stderr) + return 2 + + if findings: + for f in findings: + print(f) + print("", file=sys.stderr) + print("check-portable-atomics: std 64-bit atomics do not exist on 32-bit MIPS, so the", + file=sys.stderr) + print("crate stops building there. Use portable_atomic::AtomicU64 (or AtomicI64).", + file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/testing/ci-local.sh b/testing/ci-local.sh index c8361094..7a0aec69 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -1554,6 +1554,16 @@ run_comment_refs() { record "comment-refs" $rc } +# No non-test code may use std's 64-bit atomics. They do not exist on 32-bit +# MIPS, so one such use stops the crate building for the OpenWrt MIPS targets, +# and no leg builds for MIPS to notice. Static, and it needs nothing built. +run_portable_atomics() { + local rc=0 + info "[portable-atomics] Checking that no non-test code uses std 64-bit atomics" + python3 "$SCRIPT_DIR/check-portable-atomics.py" || rc=$? + record "portable-atomics" $rc +} + # Every daemon log string a test matches on must still be emitted by src/. # A stale one does not fail — it stops observing, and an expect-zero assertion # built on it then passes for the wrong reason. @@ -1636,6 +1646,7 @@ main() { run_image_scoping run_action_pins run_comment_refs + run_portable_atomics run_wait_converge run_deb_version run_nextest_flaky