test(multitude): drive alignment guards through an injectable cap - #704
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 587 587
Lines 62997 63131 +134
========================================
+ Hits 62997 63131 +134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes multitude’s clean-checkout cargo test failures on codegen backends that cap type alignment (e.g., 8192) by making the arena’s alignment-rejection caps injectable under cfg(test), then rewriting over-alignment tests to exercise the same guard boundaries using smaller, backend-portable alignments.
Changes:
- Add a test-only alignment cap knob on
Arenaand route all alignment guards throughrejects_smart_ptr_align/rejects_chunk_align. - Move/replace over-alignment coverage from
crates/multitude/tests/*into unit tests insrc/(including a newarena/align_guard_tests.rs) usingcapped_arena()and shared aligned helper types. - Remove the
align_capped_backendcfg wiring from the workspace.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/multitude/tests/zerocopy_integration.rs | Removes backend-gated over-alignment integration tests now covered by in-crate unit tests. |
| crates/multitude/tests/pin_support.rs | Removes the over-alignment test that depended on very large repr(align) values. |
| crates/multitude/tests/bytemuck_integration.rs | Removes backend-gated over-alignment integration tests now covered by in-crate unit tests. |
| crates/multitude/tests/audit_repro.rs | Removes backend-gated over-alignment regression coverage now covered by unit tests. |
| crates/multitude/tests/arena.rs | Removes large-alignment test types/cases and documents the one remaining >8192 aligned type. |
| crates/multitude/src/zerocopy.rs | Adds cfg(test) unit tests that drive alignment guards using capped_arena() helpers. |
| crates/multitude/src/tests_support.rs | Adds shared test-only cap constants, capped_arena(), and aligned helper types for guard testing. |
| crates/multitude/src/error.rs | Updates AllocError::is_alignment_too_large docs to avoid non-portable doctest alignment examples. |
| crates/multitude/src/bytemuck.rs | Adds cfg(test) unit tests that drive alignment guards using capped_arena() helpers. |
| crates/multitude/src/arena/mod.rs | Adds test-only alignment cap storage plus shared *_align_cap / rejects_* helpers. |
| crates/multitude/src/arena/alloc_value.rs | Replaces duplicated const caps with arena-based guard helpers for sized smart-pointer paths. |
| crates/multitude/src/arena/alloc_unsized.rs | Routes unsized/DST smart-pointer alignment checks through arena guard helpers. |
| crates/multitude/src/arena/alloc_slice_ref.rs | Routes simple-reference slice alignment checks through arena chunk-cap helper. |
| crates/multitude/src/arena/alloc_slice_box.rs | Routes boxed-slice alignment checks through arena smart-pointer-cap helper. |
| crates/multitude/src/arena/alloc_slice_arc.rs | Routes arc/rc-slice alignment checks through arena smart-pointer-cap helper. |
| crates/multitude/src/arena/align_guard_tests.rs | New unit-test suite asserting each entry point rejects alignments at/above the relevant cap. |
| crates/multitude/src/allocator_impl.rs | Uses arena-derived smart-pointer cap for allocator alignment rejection + test updates. |
| Cargo.toml | Removes align_capped_backend from the workspace check-cfg list. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 12 findings. One finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Miss |
|
Thanks for doing this. I was worried about our previous attempts to solve the problem, good to see a new attempt. Third time is the charm! |
There was a problem hiding this comment.
🟡 Changes recommended
The new test modules include unqualified align_of usage and a misleading set_align_cap panic message/expectation that should be corrected for clarity and consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
crates/multitude/src/arena/align_guard_tests.rs:32
- This
#[should_panic(expected = ...)]string should matchArena::set_align_cap’s panic message; if that message is updated to describe thecap <= CHUNK_ALIGNconstraint, update this expected substring as well.
crates/multitude/src/arena/mod.rs:597 - The assertion currently checks
cap <= CHUNK_ALIGN, but the panic message says the cap “may only be lowered” and explains a different failure mode. Updating the message to describe the actual constraint (cap must not exceedCHUNK_ALIGN) makes failures easier to diagnose.
crates/multitude/src/tests_support.rs:74
- These assertions call
align_of::<...>()without importing it in this module. Qualify the calls (or add an import within the changed region) so the const check doesn’t depend on an unshownuse.
assert!(align_of::<SmartPtrOverAligned>() == TEST_SMART_PTR_ALIGN);
assert!(align_of::<SmartPtrOverAlignedDrop>() == TEST_SMART_PTR_ALIGN);
assert!(align_of::<ChunkOverAligned>() == TEST_CHUNK_ALIGN);
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
`cargo test` failed on a clean checkout. The arena rejects allocations aligned at or above a cap: `CHUNK_ALIGN` is 64 KiB and the smart-pointer cap is half of it, 32 KiB. Testing those guards meant declaring types with `#[repr(align(32768))]` and larger, which some codegen backends refuse to compile at all — the library built fine, but the `arena`, `audit_repro` and `pin_support` test binaries and one doctest failed codegen. The previous workaround gated those tests behind a cfg that nothing in-tree sets, so the default build was the broken one, and it dropped the coverage wholesale on any backend that set it. Lower the cap to reach a legal alignment instead of raising a type's alignment to reach the cap. `Arena` gains a `cfg(test)` alignment cap that the guards read; tests set it to 8192 and drive both boundaries with 4096- and 8192-aligned types, which every backend accepts. The affected tests move in-crate as unit tests so they can reach it. Also collapses three duplicate `MAX_SMART_PTR_ALIGN` definitions into one accessor and removes the cfg entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The zerocopy scalar tests used the chunk-cap fixture, but `try_alloc` and `alloc` route through the smart-pointer guard. They could not detect that guard loosening from 4 KiB to 8 KiB. Use the smart-pointer fixture, as the bytemuck tests already did. Every other fixture sits exactly at its threshold, so narrowing either predicate from `>=` to `==` left the suite green. Add tests that lower the cap further and drive the same fixtures from strictly above it, plus the accepting counterpart below. `set_align_cap` accepted 1, which makes the derived smart-pointer cap 0 and rejects every alignment; assert a lower bound. Also documents both alignment boundaries on `is_alignment_too_large` instead of only the smart-pointer one, and drops a section marker left empty by the test migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
83b4ec5 to
a0217cc
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of concrete correctness issues in the new/updated test modules (unqualified align_of usage and one misleading helper-type doc comment) that should be addressed before merging.
Review details
Suppressed comments (3)
crates/multitude/src/tests_support.rs:75
- The const assertions use
align_ofwithout importing it; qualify these calls withcore::mem::align_of(or importalign_of) so the assertions are unambiguous.
const _: () = {
assert!(align_of::<SmartPtrOverAligned>() == TEST_SMART_PTR_ALIGN);
assert!(align_of::<SmartPtrOverAlignedDrop>() == TEST_SMART_PTR_ALIGN);
assert!(align_of::<ChunkOverAligned>() == TEST_CHUNK_ALIGN);
};
crates/multitude/src/tests_support.rs:47
- The doc comment says this type is "accepted by the simple-reference paths", but the scalar
&mut Tentry points (e.g.try_alloc::<T>()) are described elsewhere as using the smart-pointer cap. Consider clarifying that the "accepted" behavior refers specifically to the simple-reference slice entry points, which use the chunk cap.
/// Aligned exactly at the smart-pointer cap: rejected by every
/// smart-pointer entry point, accepted by the simple-reference paths.
crates/multitude/src/arena/align_guard_tests.rs:19
- This module uses
align_of::<T>()in multiple tests, but doesn’t import or qualifyalign_of. Adduse core::mem::align_of;(or fully qualify each call) to keep the tests self-contained.
use crate::Arena;
use crate::internal::constants::{CHUNK_ALIGN, max_smart_ptr_align};
use crate::tests_support::{ChunkOverAligned, SmartPtrOverAligned, SmartPtrOverAlignedDrop, TEST_CHUNK_ALIGN, capped_arena};
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ures The hand-written impls defined only_derive_is_allowed_to_implement_this_trait and is_bit_valid, which zerocopy reserves for derive-generated code and may change in a compatible release. Both fixtures are repr(C) wrappers over a u8, so the derives prove the same contracts through the supported path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change keeps production behavior intact while making alignment-guard coverage portable; remaining feedback is limited to a small doc-comment wording nit in test support helpers.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/multitude/src/tests_support.rs:66
- The doc comment for
ChunkOverAlignedsays “no chunk can satisfy it”, but undercapped_arena()this is a test-only lowered cap used to drive the guard; the underlying chunks are stillCHUNK_ALIGN-aligned, so the rejection is by design of the guard rather than a hard physical impossibility. Rewording avoids misleading future readers about what the helper is modeling.
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
The bug
ADO 7707893:
cargo testfails on a clean checkout.The arena refuses allocations whose alignment reaches a cap.
CHUNK_ALIGNis 64 KiB, and the smart-pointer cap is half of it, 32 KiB, because a smart pointer recovers its chunk header by masking the value pointer's offset within its chunk tile — a value aligned that far can land outside the first tile, where the mask finds a different chunk's header.To test the rejection, the tests had to instantiate a type aligned at or above the cap. So they declared
#[repr(align(32768))],#[repr(align(65536))]and#[repr(align(131072))]types. Some codegen backends cap type alignment at 8192 and refuse to compile such a type at all. The library built fine; three test binaries (arena,audit_repro,pin_support) and one doctest failed codegen.Why the previous fix didn't hold
#501 gated the tests behind
#[cfg(not(utc_backend))](since renamedalign_capped_backend), with the flag set by an out-of-tree CI pipeline. Three problems:RUSTFLAGS. SettingRUSTFLAGSwould also have clobbered.cargo/config.toml's-C target-cpu=x86-64-v3.This change
The test needs the type's alignment and the cap to meet. The old approach raised the alignment to the cap. This one lowers the cap to an alignment every backend compiles.
Arenagets a#[cfg(test)]alignment cap that the guards read:Tests call
capped_arena(), which sets the cap to 8192, and use shared helper types aligned to 4096 and 8192. Both boundaries stay reachable, and the production 2:1 ratio between the chunk cap and the smart-pointer cap is preserved, so each test still exercises the cap its entry point actually consults.The
cfg(test)field and the whole knob disappear from production builds —Arena's layout is unchanged.Along the way:
MAX_SMART_PTR_ALIGNconstants collapse into one accessor. All nine guard sites now route throughrejects_smart_ptr_align/rejects_chunk_align.tests/into#[cfg(test)]modules insrc/(arena/align_guard_tests.rs,bytemuck.rs,zerocopy.rs) so they can reach the knob.align_capped_backendcfg is deleted from the workspaceCargo.toml.Coverage
Nothing was dropped. Two additions beyond parity:
is_alignment_too_large(), which nothing outside the deleted doctest checked before.try_alloc_slice_fill_iter's guard had no over-alignment test at all; it does now.Mutation-checked by hand: forcing
rejects_smart_ptr_aligntofalsefails 39 tests, forcingrejects_chunk_aligntofalsefails 9.Things worth a reviewer's attention
The guards are no longer
const { }. They wereif const { align_of::<T>() >= MAX_SMART_PTR_ALIGN }, folded at compile time by construction. They are now ordinary comparisons against an#[inline(always)]accessor. In release undercfg(not(test))that accessor returns a literal andalign_of::<T>()is a constant, so LLVM folds it; debug builds pay a compare. The guarantee is gone, the behaviour isn't. Restoring the guarantee would need a macro expanding to theconst { }form undercfg(not(test))— happy to add it if you'd rather have the certainty.buffer_freezablestill reads the real cap. It's used insideconst { }on theVechot path, so making it cap-aware would put a runtime branch there. The consequence is that a capped arena is not a faithful model forVec/Stringgrowth and freeze tests — documented oncapped_arena()andset_align_cap(), andset_align_capnow asserts the cap can only be lowered. A new lib test asserts the arena's default caps equalCHUNK_ALIGNandmax_smart_ptr_align(), so the two sources can't drift apart silently.One over-aligned type survives.
non_freezable_overaligned_vec_grows_via_oversized_pathintests/arena.rsstill declares#[repr(align(32768))]. It's the one place where the alignment is the subject — it's what makes the element non-freezable — and it compiles becausetry_reservenever materialises the layout. That's an emergent property, not a guarantee, so there's now a comment naming it as the one fragile declaration left, to make a future failure diagnosable.Verification
cargo test -p multitude --all-featurespasses on both the alignment-capped backend and an LLVM-backend toolchain.cargo clippy -p multitude --all-features --all-targets -- -D warningsclean.cargo spellcheckcould not be run — the binary is broken in my environment (missing DLL). Please let CI cover it.Not fixed here
A clean-checkout
cargo build --workspaceon the internal toolchain also fails inzeroize1.9.0 (reached viafetch*→rustls→aws-lc-rs) withcodegen not yet implemented for Terminator_InlineAsm. Unrelated to alignment and not fixable in this repo. Worth tracking separately.