test(multitude): drive alignment guards through an injectable cap - #704
test(multitude): drive alignment guards through an injectable cap#704Adomas Bekeras (AdomasBekeras) wants to merge 2 commits into
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 63121 +124
========================================
+ Hits 62997 63121 +124
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.
| assert!(align_of::<SmartPtrOverAligned>() == TEST_SMART_PTR_ALIGN); | ||
| assert!(align_of::<SmartPtrOverAlignedDrop>() == TEST_SMART_PTR_ALIGN); | ||
| assert!(align_of::<ChunkOverAligned>() == TEST_CHUNK_ALIGN); |
| /// Aligned exactly at the smart-pointer cap: rejected by every | ||
| /// smart-pointer entry point, accepted by the simple-reference paths. | ||
| #[derive(Clone, Copy, Debug)] |
| assert!(cap.is_power_of_two(), "alignment cap must be a power of two"); | ||
| assert!( | ||
| cap <= CHUNK_ALIGN, | ||
| "the cap may only be lowered: raising it would let the guards accept alignments no chunk can satisfy" | ||
| ); |
| /// Such a request can never | ||
| /// succeed, regardless of how much memory is available. | ||
| /// Such a request can never succeed, regardless of how much memory is | ||
| /// available. The arena rejects any value whose alignment reaches half |
There was a problem hiding this comment.
🤖: Non-blocking — is_alignment_too_large presents the smart-pointer cap as universal. Document both boundaries: smart-pointer and single-value allocations reject at 32 KiB; simple-reference slices reject at 64 KiB.
| #[test] | ||
| fn try_alloc_ref_over_aligned_returns_err() { | ||
| let arena = capped_arena(); | ||
| arena.zerocopy().try_alloc::<ChunkOverAligned>().unwrap_err(); |
There was a problem hiding this comment.
🤖: Non-blocking — these scalar zerocopy tests use the chunk-cap fixture, so they cannot detect a guard loosened from 4 KiB to 8 KiB. Use SmartPtrOverAligned in try_alloc_ref_over_aligned_returns_err and alloc_ref_panics_on_over_aligned, matching the bytemuck tests.
|
|
||
| #[cfg(feature = "zerocopy")] | ||
| // SAFETY: a `u8` plus alignment padding; every byte pattern is valid. | ||
| unsafe impl zerocopy::TryFromBytes for $ty { |
There was a problem hiding this comment.
🤖: Non-blocking — these manual zerocopy implementations bypass the traits' derive-only seal and hidden internals. Replace the zerocopy arms of impl_zero_valid! with feature-gated derives on the fixture types. The current u8 fixtures are bit-valid, but derives prevent this generic macro from silently asserting validity for a future niche-bearing type.
There was a problem hiding this comment.
[Copilot speaking]
Derive the bytemuck and zerocopy marker traits instead of implementing their internals
The new shared alignment fixtures manually implement bytemuck::Zeroable, zerocopy::TryFromBytes, and zerocopy::FromZeros. The zerocopy implementations define TryFromBytes::is_bit_valid and call hidden only_derive_is_allowed_to_implement_this_trait methods that its public documentation explicitly reserves for derive-generated code, even though this crate enables both dependencies' derive features and declares the fixtures in this module.
Reproducible reasoning: crates/multitude/Cargo.toml enables bytemuck and zerocopy with their derive features, and Cargo.lock selects zerocopy 0.8.52. SmartPtrOverAligned and ChunkOverAligned are repr(C) wrappers containing only a u8, so the supported derives can prove their marker contracts directly. Zerocopy's documented contract directs implementers to derive TryFromBytes, permits callers to trust a successful validity result, and warns that hidden internals can change in otherwise compatible releases in ways that affect soundness. FromZeros extends TryFromBytes, and deriving FromZeros emits both implementations after checking the fields. Although the handwritten true validity result is reasonable for these fixtures, the macro unnecessarily assumes ownership of unsafe contracts and bypasses the dependencies' supported compile-time analysis.
Consequence: A routine zerocopy dependency update can change hidden trait machinery or validity requirements while these handwritten implementations either stop compiling or continue with an outdated proof of bit validity, potentially making the test-only code unsound and blocking dependency updates.
Recommended action: Apply conditional derives to both fixture structs, using cfg_attr(feature = "bytemuck", derive(bytemuck::Zeroable)) and cfg_attr(feature = "zerocopy", derive(zerocopy::FromZeros)), then remove impl_zero_valid and its handwritten marker implementations. This uses the dependencies' supported compile-time-checked path; deriving zerocopy's FromZeros also generates the required TryFromBytes implementation.
References:
- crates/multitude/Cargo.toml
- bytemuck 1.25.0 Zeroable derive
- zerocopy 0.8.26 FromZeros implementation contract
- zerocopy 0.8.26 TryFromBytes implementation contract
- Zerocopy 0.8.52 TryFromBytes implementation contract
Impacted locations:
crates/multitude/src/tests_support.rs:77-105crates/multitude/src/tests_support.rs:87-100
Sander Saares (sandersaares)
left a comment
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 |
| // `repr(align)` takes a literal, so the helper types cannot name the cap | ||
| // constants directly. Keep them in step. |
There was a problem hiding this comment.
[Copilot speaking]
Replace the colloquial maintenance instruction with an explicit statement
The comment explaining the compile-time alignment assertions ends with “Keep them in step.” This idiom leaves both the antecedent of “them” and the precise maintenance requirement implicit, even though the following assertions enforce a specific relationship between the literal type alignments and the cap constants.
Reproducible reasoning: The English prose guideline requires professional language and explicitly asks authors to avoid colloquialisms. “Keep them in step” is an idiomatic expression rather than a direct description of the invariant. Reading the following assertions shows that the intended instruction is to keep the literal repr(align) values synchronized with TEST_SMART_PTR_ALIGN and TEST_CHUNK_ALIGN, so the comment can state that purpose directly.
Consequence: A maintainer, especially one unfamiliar with the idiom, may need to inspect and reverse-engineer the assertions to determine which values must remain synchronized when changing the test cap.
Recommended action: Replace the final sentence with direct wording such as: “The assertions below keep the literal type alignments synchronized with the cap constants.”
Impacted locations:
crates/multitude/src/tests_support.rs:69-70
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
[Copilot speaking]
Run the relocated bytemuck and zerocopy tests in default test builds
The relocated bytemuck and zerocopy alignment tests are nested in source modules that remain included only under their Cargo features. The Arena accessors and test fixture marker implementations they use are likewise feature-gated. Because Rust evaluates these enclosing gates before discovering the nested #[cfg(test)] modules, an ordinary cargo test build excludes the relocated coverage even though the optional crates are available as dev-dependencies.
Reproducible reasoning: A unit-test module can run only when every enclosing module and API it uses is compiled. crates/multitude/src/lib.rs excludes the bytemuck and zerocopy modules unless their features are active, crates/multitude/src/arena/mod.rs similarly excludes the corresponding Arena accessors, and crates/multitude/src/tests_support.rs excludes the required marker implementations. The inner #[cfg(test)] attributes therefore cannot make these tests participate in a default test build. This conflicts with the repository convention of making feature-dependent code available under cfg(any(test, feature = "...")) when dev-dependencies supply the optional crates.
Consequence: Regressions in the bytemuck or zerocopy alignment adapters can pass the ordinary test build; the relocated coverage runs only when callers explicitly enable the optional features or use all features.
Recommended action: Include the bytemuck and zerocopy module declarations and their Arena accessors under cfg(any(test, feature = "...")), while retaining feature-only doc(cfg(...)) annotations for published API documentation. Make the test-only fixture marker implementations available unconditionally within tests_support.rs, or use the matching any(test, feature = "...") conditions. Keep both optional crates available as non-optional dev-dependencies and enable zerocopy's derive feature for the dev-dependency as for the regular dependency.
References:
- crates/multitude/Cargo.toml feature and dev-dependency declarations
Impacted locations:
crates/multitude/src/bytemuck.rs:454-455crates/multitude/src/zerocopy.rs:451-452crates/multitude/src/tests_support.rs:83-99crates/multitude/src/lib.rs:518-529crates/multitude/tests/bytemuck_integration.rs:151-230crates/multitude/tests/zerocopy_integration.rs:124-196crates/multitude/src/bytemuck.rs:455-536crates/multitude/src/zerocopy.rs:452-533crates/multitude/src/lib.rs:518-529crates/multitude/src/arena/mod.rs:516-542crates/multitude/Cargo.toml:64-100
| #[test] | ||
| fn try_alloc_slice_box_over_aligned_returns_err() { | ||
| let arena = capped_arena(); | ||
| arena.bytemuck().try_alloc_slice_box::<SmartPtrOverAligned>(4).unwrap_err(); |
There was a problem hiding this comment.
[Copilot speaking]
Explain the non-empty slice length used by the adapter guard tests
The new bytemuck and zerocopy slice rejection tests repeatedly use the element count 4, although the tested condition depends on element alignment and only needs a non-empty slice to select the initialization path. The test modules do not say whether that count is significant or arbitrary.
Reproducible reasoning: Each affected test changes the allocation family and aligned fixture while keeping the count fixed. The guard executes from align_of::<T>() before allocation size can affect the result, so no assertion depends on four elements. A reader extending the matrix must otherwise inspect the guard implementations to learn that any ordinary non-empty count is equivalent for this purpose.
Consequence: Future cases can preserve or change the repeated count inconsistently, and reviewers must reverse engineer whether slice length participates in the boundary being tested.
Recommended action: Use the simplest non-empty witness consistently, such as one element, and document once near the shared adapter matrix that slice length is intentionally minimal because only element alignment is under test. Introduce a named constant only if the length later gains semantics beyond being non-empty.
Impacted locations:
crates/multitude/src/bytemuck.rs:481-541crates/multitude/src/zerocopy.rs:470-530
| /// Smart-pointer alignment cap under [`TEST_CHUNK_ALIGN`]. Half the chunk | ||
| /// cap, as in production. |
There was a problem hiding this comment.
[Copilot speaking]
Condense the new alignment-related documentation summaries
Several new item documentation blocks combine their summary and detailed rationale in one opening paragraph, so their rendered first paragraphs exceed the repository's 80-character summary limit. This affects TEST_SMART_PTR_ALIGN, SmartPtrOverAlignedDrop, ChunkOverAligned, the test-only align_cap field, and set_align_cap.
Reproducible reasoning: Rustdoc joins adjacent documentation-comment lines into one paragraph; wrapping the source therefore does not create a short rendered summary. Each affected item can state its role in one sentence, while details about production geometry, test-only thresholds, and needs_drop branching belong in a later paragraph. The change improves generated-documentation scanning without removing the rationale.
Consequence: Readers scanning item documentation receive wrapped implementation detail before a concise statement of each item's role.
Recommended action: Give each affected item a rendered opening sentence of at most 80 characters, add a blank documentation line, and move the remaining explanation into later paragraphs. Keep the substantive distinction between production geometry and synthetic test thresholds while shortening the summaries.
Impacted locations:
crates/multitude/src/tests_support.rs:30-31crates/multitude/src/tests_support.rs:52-53crates/multitude/src/tests_support.rs:63-64crates/multitude/src/arena/mod.rs:158-160crates/multitude/src/arena/mod.rs:583-585
| #[test] | ||
| #[should_panic(expected = "the cap may only be lowered")] | ||
| fn set_align_cap_rejects_raising_the_cap() { | ||
| Arena::new().set_align_cap(CHUNK_ALIGN * 2); |
There was a problem hiding this comment.
[Copilot speaking]
Explain why the raising-cap test preserves power-of-two alignment
The new set_align_cap_rejects_raising_the_cap test passes CHUNK_ALIGN * 2, but it does not document that this particular relationship is required to reach the intended upper-bound assertion. set_align_cap first rejects values that are not powers of two, so an apparently equivalent above-cap value can exercise a different validation branch.
Reproducible reasoning: The test expects the panic text from the cap <= CHUNK_ALIGN assertion. The preceding assertion requires cap.is_power_of_two(). Multiplying the power-of-two production cap by another power of two preserves that prerequisite while exceeding the production cap. This dependency is a non-obvious reason for the chosen literal multiplier and should be recorded where the value is selected.
Consequence: A maintainer can replace the expression with a simpler-looking above-cap value such as an incremented cap and unintentionally turn this into a test of the power-of-two check, weakening coverage of the lower-only constraint.
Recommended action: Add an inline comment explaining that the test input must remain a power of two while exceeding the production cap so execution reaches the lower-only assertion. Describe the relationship without restating the literal value.
Impacted locations:
crates/multitude/src/arena/align_guard_tests.rs:34
| // | ||
| // This is the last type in the crate aligned above 8192, which some | ||
| // codegen backends refuse to compile (see the arena's alignment-guard | ||
| // unit tests for why the others were removed). It compiles only because | ||
| // `try_reserve` never materializes the layout. If this file ever fails to | ||
| // build with an alignment error, this declaration is the cause. | ||
| #[test] |
There was a problem hiding this comment.
[Copilot speaking]
Explain the remaining high-alignment reserve test accurately and locally
The new comment above non_freezable_overaligned_vec_grows_via_oversized_path describes removed declarations, calls this the crate's last type above a backend threshold, gives an absolute diagnosis for any future alignment build failure, and says try_reserve never materializes the layout. The durable local fact is narrower: the high alignment makes Over non-freezable and selects the oversized reserve path; the code computes its size and alignment and reserves aligned raw storage, but it never constructs an Over value or an over-aligned stack temporary.
Reproducible reasoning: Vec::try_grow_to uses size_of::<T>(), align_of::<T>(), and the non-freezable reservation path, so layout information is part of the operation. The test avoids push and leaves the vector empty, which means no typed Over value is created on the stack or written into the reserved storage. That distinction explains the intended portability behavior without maintaining a repository-wide census, narrating removed tests, or claiming every possible future alignment diagnostic has one cause.
Consequence: The current comment can become stale when other fixtures are added or moved and can send a maintainer toward allocation-layout construction even though the test deliberately preserves it and avoids only typed value materialization.
Recommended action: Replace the historical census and absolute troubleshooting claim with a present-state comment: the alignment intentionally makes the element non-freezable and drives oversized reservation; the path still computes and reserves aligned storage but does not construct an Over value or over-aligned stack temporary. Keep any backend caveat scoped to that typed-materialization constraint.
References:
- crates/multitude/src/vec/mod.rs (
Vec::try_grow_to)
Impacted locations:
crates/multitude/tests/arena.rs:3777-3801crates/multitude/src/vec/mod.rs:129-183
There was a problem hiding this comment.
[Copilot speaking]
Remove the now-empty Helpers section marker
The change removes the only helper type declarations beneath the three-line Helpers separator in coverage_arena_gaps, leaving the separator immediately followed by ordinary tests. The marker therefore no longer describes any content in the module.
Reproducible reasoning: Before this change, HalfChunkAlign and ChunkAlign appeared directly beneath the marker. The pull request deletes both declarations but retains the visual chapter heading. Reading the resulting module shows that the first item after the marker is try_alloc_simple_ref_returns_mutable_reference, so there is no helper section to identify. This also conflicts with the code-ordering guidance to avoid section-separator comments and instead make organization clear through code structure.
Consequence: Readers are directed to expect shared helper definitions and must scan onward to discover that the advertised section is empty, adding noise and making the module's organization misleading.
Recommended action: Delete the Helpers label and its surrounding separator lines. No replacement heading is needed because the remaining items are tests in the existing test module.
Impacted locations:
crates/multitude/tests/arena.rs:2236-2238
| /// Exclusive upper bound on the alignment a chunk can satisfy. | ||
| /// | ||
| /// Production builds always report [`CHUNK_ALIGN`]. Tests lower it | ||
| /// with [`Self::set_align_cap`] so the rejection guards can be driven | ||
| /// by types whose alignment every codegen backend accepts — a type | ||
| /// aligned to the real cap does not compile everywhere. |
There was a problem hiding this comment.
[Copilot speaking]
Define one authoritative alignment-policy model and use it consistently
The change uses the word "cap" for three different concepts whose relationships are not defined in one place: immutable production geometry (CHUNK_ALIGN and the first-tile masking invariant), test-only synthetic rejection thresholds lowered by set_align_cap, and the API-specific limits selected by each allocation family. The resulting documentation is internally inconsistent. Escape-capable Box, Rc, and Arc owners use the lower smart-pointer threshold; arena-bound Alloc<[T]> slices use the full chunk threshold; scalar Alloc<T> currently uses the lower threshold even though the design says arena-lifetime allocations are not subject to the smart-pointer cap; and all of these failures share AllocError::is_alignment_too_large. Local comments also describe synthetic thresholds as physical geometry and call Alloc<[T]> results plain references.
Reproducible reasoning: Production chunks remain aligned to CHUNK_ALIGN, and smart-pointer destruction recovers a chunk header by masking the payload address. The new cfg(test) field does not change that geometry: it only changes the values read by rejects_smart_ptr_align and rejects_chunk_align. Call-site inspection shows that smart owners and scalar Alloc<T> call the lower-threshold predicate, while arena-bound slice helpers call the full-chunk predicate; buffer_freezable remains a constant expression over the production threshold. The public error classifier therefore represents whichever limit applies to the selected API rather than one universal numeric boundary. Because no authoritative model names these layers and maps owner families to their policy, the helper rustdoc, test overview, public error docs, and design document each generalize from a different subset of the behavior. The scalar Alloc<T> call site additionally exposes a real unresolved contract question: its lower threshold is established behavior, but the nearby masking explanation applies to escape-capable owners and the design explicitly describes arena-lifetime allocations differently.
Consequence: Maintainers can use a synthetic test threshold in geometry-dependent code, apply the wrong guard to a new allocation entry point, or change the scalar Alloc<T> limit based on contradictory documentation. Callers can also infer a universal half-chunk limit from the shared error classifier even when an arena-bound slice API accepts alignments up to the full chunk threshold.
Recommended action: Create an affected-scope implementation section that defines production geometry, synthetic test thresholds, and API alignment policy as separate concepts. Include a table mapping scalar Alloc<T>, arena-bound Alloc<[T]>, and escape-capable owners to their selected threshold and rationale; explicitly decide and document the scalar Alloc<T> policy without presuming whether code or documentation should change. Record why the portable cases are crate unit tests and why buffer_freezable remains production-constant-based. Link this model from the existing design sections, then align the public error documentation and local comments with it: describe the shared classifier generically, use the established Alloc terminology, scope masking and chunk-capacity claims to production geometry, and describe the test override in present-state terms.
References:
- crates/multitude/docs/DESIGN.md, "Thin smart pointers: the alignment/masking trick", "Fragile invariants", and "Testing and verification"
- crates/multitude/docs/DESIGN.md, "Thin smart pointers: the alignment/masking trick" and "Sharp edges for users"
- crates/multitude/docs/DESIGN.md, "Thin smart pointers: the alignment/masking trick" and "The four allocation styles"
- crates/multitude/docs/DESIGN.md, "The four allocation styles"
- crates/multitude/docs/DESIGN.md, "The four allocation styles", "Thin smart pointers: the alignment/masking trick", and "Sharp edges for users"
- crates/multitude/docs/DESIGN.md, "Thin smart pointers: the alignment/masking trick"
Impacted locations:
crates/multitude/docs/DESIGN.mdcrates/multitude/src/tests_support.rs:21-42crates/multitude/src/arena/align_guard_tests.rs:4-28crates/multitude/src/arena/mod.rs:554-626crates/multitude/tests/arena.rs:3777-3801crates/multitude/src/error.rs:93-102crates/multitude/src/bytemuck.rs:91-129crates/multitude/src/zerocopy.rs:88-126crates/multitude/src/arena/align_guard_tests.rs:4-15crates/multitude/src/arena/align_guard_tests.rs:21-23crates/multitude/src/arena/align_guard_tests.rs:134-156crates/multitude/src/arena/alloc_value.rs:794-800crates/multitude/src/arena/mod.rs:601-616crates/multitude/src/arena/alloc_value.rs:916-919crates/multitude/src/arena/mod.rs:563-626
There was a problem hiding this comment.
[Copilot speaking]
Separate guard semantics from entry-point routing in the portable tests
The portable alignment tests are maintained as flat, hand-written API examples, although the change introduces two distinct verification responsibilities: proving the centralized predicates' comparison semantics and proving that every independently implemented allocation route calls the correct predicate before any fast return. The current suite tests only equality at both exclusive thresholds, omits distinct Box-copy, arena-bound clone, and DST guard implementations, no longer preserves typed-empty rejection coverage for several slice paths, and duplicates the bytemuck and zerocopy adapter matrix closely enough that the zerocopy scalar cases already use the wrong boundary fixture.
Reproducible reasoning: rejects_smart_ptr_align and rejects_chunk_align implement align >= threshold; all existing portable rejection fixtures are exactly equal to the injected thresholds, so changing either predicate to equality would preserve the suite. Call-site inspection then reveals independently breakable routes: Box copy calls check_slice_box_layout separately from Box fill, arena-bound clone calls reject_over_aligned separately from copy and fill, Box DST has its own runtime-layout guard, and Arc/Rc DST share another guard. These sites are not reached with the lowered cap. The slice guards execute before their zero-length fast returns, but relocated copy tests use non-empty inputs and the clone acceptance test uses a fixture below that call site's threshold, so those tests cannot detect a disconnected guard or reordered empty return. Finally, bytemuck and zerocopy scalar adapters both forward to Arena::try_alloc_with, yet only the bytemuck copy uses the lower-threshold fixture; the duplicated matrices therefore fail to encode their shared routing contract structurally. These are consequences of the same test organization rather than independent API defects.
Consequence: A predicate can regress from >= to ==, a distinct allocation route can stop consulting the injected threshold, an empty slice can bypass validation, or one ecosystem adapter can drift to the wrong boundary while all portable tests that currently exercise the affected layer remain green.
Recommended action: Organize the portable coverage in two layers. First, add focused truth-table tests for both centralized predicates at equality and strictly above the injected threshold, without duplicating the full API matrix. Second, define an explicit routing matrix that maps every distinct guard implementation to its expected threshold fixture and input shape. Include typed-empty cases where validation must precede a fast return, cover Box copy and arena-bound clone separately, exercise both DST guard implementations with matching sized layouts under the dst feature, and generate the equivalent bytemuck and zerocopy cases from one shared mapping after correcting the zerocopy scalar fixture to SmartPtrOverAligned. This finding concerns semantic coverage when the relevant modules are compiled; feature-selection coverage remains a separate concern.
References:
- crates/multitude/src/arena/alloc_value.rs
- crates/multitude/docs/DESIGN.md, "Thin smart pointers: the alignment/masking trick" and "Safety invariants"
- crates/multitude/src/arena/align_guard_tests.rs
- crates/multitude/tests/dst.rs
Impacted locations:
crates/multitude/src/bytemuck.rs:451-543crates/multitude/src/zerocopy.rs:451-532crates/multitude/src/arena/align_guard_tests.rs:13-15crates/multitude/src/arena/align_guard_tests.rs:54-156crates/multitude/src/arena/mod.rs:609-626crates/multitude/src/arena/align_guard_tests.rscrates/multitude/src/arena/alloc_slice_box.rs:231-232crates/multitude/src/arena/alloc_slice_ref.rs:374-375crates/multitude/src/arena/alloc_unsized.rs:390-392crates/multitude/src/arena/alloc_unsized.rs:479-481
|
|
||
| #[cfg(feature = "zerocopy")] | ||
| // SAFETY: a `u8` plus alignment padding; every byte pattern is valid. | ||
| unsafe impl zerocopy::TryFromBytes for $ty { |
There was a problem hiding this comment.
[Copilot speaking]
Derive the bytemuck and zerocopy marker traits instead of implementing their internals
The new shared alignment fixtures manually implement bytemuck::Zeroable, zerocopy::TryFromBytes, and zerocopy::FromZeros. The zerocopy implementations define TryFromBytes::is_bit_valid and call hidden only_derive_is_allowed_to_implement_this_trait methods that its public documentation explicitly reserves for derive-generated code, even though this crate enables both dependencies' derive features and declares the fixtures in this module.
Reproducible reasoning: crates/multitude/Cargo.toml enables bytemuck and zerocopy with their derive features, and Cargo.lock selects zerocopy 0.8.52. SmartPtrOverAligned and ChunkOverAligned are repr(C) wrappers containing only a u8, so the supported derives can prove their marker contracts directly. Zerocopy's documented contract directs implementers to derive TryFromBytes, permits callers to trust a successful validity result, and warns that hidden internals can change in otherwise compatible releases in ways that affect soundness. FromZeros extends TryFromBytes, and deriving FromZeros emits both implementations after checking the fields. Although the handwritten true validity result is reasonable for these fixtures, the macro unnecessarily assumes ownership of unsafe contracts and bypasses the dependencies' supported compile-time analysis.
Consequence: A routine zerocopy dependency update can change hidden trait machinery or validity requirements while these handwritten implementations either stop compiling or continue with an outdated proof of bit validity, potentially making the test-only code unsound and blocking dependency updates.
Recommended action: Apply conditional derives to both fixture structs, using cfg_attr(feature = "bytemuck", derive(bytemuck::Zeroable)) and cfg_attr(feature = "zerocopy", derive(zerocopy::FromZeros)), then remove impl_zero_valid and its handwritten marker implementations. This uses the dependencies' supported compile-time-checked path; deriving zerocopy's FromZeros also generates the required TryFromBytes implementation.
References:
- crates/multitude/Cargo.toml
- bytemuck 1.25.0 Zeroable derive
- zerocopy 0.8.26 FromZeros implementation contract
- zerocopy 0.8.26 TryFromBytes implementation contract
- Zerocopy 0.8.52 TryFromBytes implementation contract
Impacted locations:
crates/multitude/src/tests_support.rs:77-105crates/multitude/src/tests_support.rs:87-100
|
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
| 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}; |
`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
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.