phase 16: no-alloc CI gate#94
Conversation
…panic-allocator witness Add two no-alloc handle types in src/transport.rs (bare_metal feature): - StaticE2EHandle: wraps &'static embassy-sync critical-section Mutex<RefCell<E2ERegistry>>; implements E2ERegistryHandle without any heap allocation on the hot path. - AtomicInterfaceHandle: wraps &'static AtomicU32; encodes IPv4 as u32; implements InterfaceHandle with single atomic load/store. Both types are Clone+Copy via thin-pointer semantics and satisfy Clone+Send+Sync+'static without Arc or RwLock. Add tests/no_alloc_witness.rs (harness=false) with a PanicAllocator #[global_allocator] that panics on any heap allocation while armed. Witnesses: AtomicInterfaceHandle get/set, StaticE2EHandle contains_key/ protect/check after construction-time register, and WitnessChannels oneshot warm claim+send — all verified alloc-free under the panic harness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JustinKovacich
changed the title
drop unsafe Send/Sync, abort-on-alloc instead of panic CI: - Add explicit `cargo test --test no_alloc_witness` step in ci.yml so the no-alloc gate is visible in CI logs (independent of nextest's handling of harness=false binaries). src/transport.rs: - Remove the redundant `unsafe impl Send`/`unsafe impl Sync` blocks on StaticE2EHandle and AtomicInterfaceHandle. Auto-traits derive correctly through `&'static T` since BlockingMutex<CSRawMutex, RefCell<E2ERegistry>> is Sync and AtomicU32 is Sync. - Document why AtomicInterfaceHandle uses Ordering::Relaxed (single synchronized datum, no happens-before to establish). - Add a "No-allocator targets" note pointing at StaticCell::init for the eventual const-friendly E2ERegistry::new path. tests/no_alloc_witness.rs: - Replace `panic!` in PanicAllocator with `diagnose_and_abort`, which disarms first then aborts via process::abort() — keeps the diagnostic off the panic-unwind path (whose machinery also allocates). - Add witness_static_channels_first_claim using a fresh `(u64, 4)` oneshot variant — proves first-claim seeds the free-list alloc-free, the case that runs once at boot on a real bare-metal target. - Add witness_static_channels_oneshot_recv — polls the recv future once via Waker::noop so the channel's poll path is measured without an executor. - Add Profile5 protect/check round-trip witness (data_length matched to payload length to avoid the tracing::warn! mismatch path). - Rewrite the `harness = false` comment to explain the actual reason (libtest TLS, worker pool, per-test bookkeeping all allocate before main() runs).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// `static`-cell pattern (e.g. `static_cell::StaticCell::init`) once the | ||
| /// registry constructor becomes `const`-friendly. The handle layer itself | ||
| /// never allocates — only the one-time storage materialization does. | ||
| #[cfg(feature = "bare_metal")] |
There was a problem hiding this comment.
bare_metal_handle_impls depends on crate::e2e::E2ERegistry, but E2ERegistry is only exported when feature = "std" (see src/e2e/mod.rs). With --no-default-features --features bare_metal, this module will fail to compile even if these handles aren’t used. Consider gating this module (and its re-exports) behind cfg(all(feature = "bare_metal", feature = "std")), or redesigning the storage alias/handle to not require the std-only E2ERegistry type.
| #[cfg(feature = "bare_metal")] | |
| #[cfg(all(feature = "bare_metal", feature = "std"))] |
| OneshotCancelled, OneshotRecv, OneshotSend, ReceivedDatagram, SocketOptions, Spawner, Timer, | ||
| TransportError, TransportFactory, TransportSocket, UnboundedRecv, UnboundedSend, | ||
| }; | ||
| #[cfg(feature = "bare_metal")] |
There was a problem hiding this comment.
These re-exports are gated only on feature = "bare_metal", but the newly added handle implementations currently rely on std-only E2ERegistry. If the implementation is updated to cfg(all(feature = "bare_metal", feature = "std")) (or otherwise made std-free), this pub use should match to avoid build failures under --no-default-features --features bare_metal.
| #[cfg(feature = "bare_metal")] | |
| #[cfg(all(feature = "bare_metal", feature = "std"))] |
| /// # Memory ordering | ||
| /// | ||
| /// Both `get` and `set` use [`Ordering::Relaxed`]. The address is the |
There was a problem hiding this comment.
unsafe impl Send/Sync here looks unnecessary and weakens the compiler’s thread-safety checks. AtomicInterfaceHandle contains only a &'static AtomicU32, which is already Send + Sync, so the auto-traits should apply without unsafe. Prefer removing these unsafe impls (or, if you keep them, add a stronger safety justification than “&'static … is already Send + Sync”).
|
|
||
| impl StaticE2EHandle { | ||
| /// Wraps a static reference to the backing mutex. | ||
| pub const fn new(storage: &'static StaticE2EStorage) -> Self { |
There was a problem hiding this comment.
unsafe impl Send/Sync is a significant safety escape hatch. It would be safer to rely on auto-traits (if StaticE2EStorage is actually Send + Sync) rather than forcing Send/Sync manually. If the underlying embassy_sync::blocking_mutex::Mutex<..., RefCell<_>> is intentionally not Send/Sync on some targets, these unsafe impls could be unsound; consider removing them or switching to a backing type that is explicitly Send + Sync.
| // SAFETY: forwarding to System; invariants upheld by caller. | ||
| unsafe { System.realloc(ptr, layout, new_size) } | ||
| } | ||
| } | ||
|
|
||
| #[global_allocator] |
There was a problem hiding this comment.
assert_no_alloc leaves the allocator armed if f() panics (including the expected “allocation forbidden” panic). That can trigger cascading panics during unwinding/panic-hook printing and make failures harder to interpret. Consider disarming via an RAII guard (Drop) or wrapping f in std::panic::catch_unwind to ensure ARMED is always reset before the panic propagates.
— StaticE2EHandle, AtomicInterfaceHandle panic-allocator witness
Add two no-alloc handle types in src/transport.rs (bare_metal feature):
Both types are Clone+Copy via thin-pointer semantics and satisfy Clone+Send+Sync+'static without Arc or RwLock.
Add tests/no_alloc_witness.rs (harness=false) with a PanicAllocator #[global_allocator] that panics on any heap allocation while armed. Witnesses: AtomicInterfaceHandle get/set, StaticE2EHandle contains_key/ protect/check after construction-time register, and WitnessChannels oneshot warm claim+send — all verified alloc-free under the panic harness.