fix(align_up): assert power-of-2 and overflow invariants

[nethum529]

Problem

align_up's power-of-2 alignment precondition was only checked via a bare assert, and there was no overflow check at all. A value near SIZE_MAX silently wrapped to a small or zero result instead of being caught (observed while debugging #2002).

Fix

• Add an overflow guard: assert(aligned_value >= value) catches wraparound in debug builds.
• Document the alignment and non-overflow invariants on align_up (and the power-of-2 requirement on align_down).
• align_up stays noexcept. Per review, it must remain callable from destructors, so the invariants are enforced with assert (debug-only) rather than by throwing. This keeps the release ABI and the destructor-safe contract intact.
• align_down is unchanged: it only clears bits, so it cannot overflow.

Tests

New gtests in ALIGNED_TEST cover the overflow boundary (AlignUpJustBelowOverflowBoundarySucceeds, AlignUpAtOverflowBoundaryIsNoOp, AlignUpAlignmentOfOneIsNoOp) and a debug-only death test (AlignedDeathTest.AlignUpOverflowAborts) that exercises the overflow guard directly. Suite passes locally.

closes #2005

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[PointKernel]

/ok to test 5d83fc3

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

rmm::align_up now documents its alignment and overflow invariants, removes noexcept, checks for wraparound with an assertion, and adds boundary tests. align_down documentation also clarifies its power-of-two requirement.

Changes

align_up overflow assertion

Layer / File(s)	Summary
Alignment contract and overflow guard cpp/include/rmm/aligned.hpp, cpp/src/aligned.cpp	Documents alignment and non-overflow requirements, removes noexcept, and asserts that the aligned result remains greater than or equal to the input.
Boundary and no-op alignment tests cpp/tests/mr/aligned_mr_tests.cpp	Tests overflow-edge values, the safe boundary, and alignment by one, including SIZE_MAX and a debug-only death test.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: harrism, pointkernel

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	[#2005] The PR only adds debug asserts for the invariant, so invalid inputs can still wrap in release builds instead of failing loudly.	Add an always-on contract check such as RMM_EXPECTS for align_up's precondition and overflow guard, not just debug-only assert handling.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes stay focused on align_up docs, implementation, and tests, with no obvious unrelated work.
Title check	✅ Passed	The title matches the main change: hardening align_up with overflow and alignment invariant checks.
Description check	✅ Passed	The description is clearly about the same align_up overflow and alignment fix, so it is related to the changeset.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[PointKernel]

/ok to test ad861de

[bdice]

This is a helpful change. I have a couple of requests.

(I realize this is an agent's contribution, so if I don't hear back, I'll push a fix myself when time allows.)

[harrism]

Sorry for the drive-by, but I think alignup is probably noexcept for a reason: so it can be called from destructors.

[nethum529]

Looking into the feedback now. Agreed on the noexcept point, align_up should stay noexcept so it can be called from destructors, so I'll keep it noexcept and use assert-based checks instead of throwing. Update coming shortly.

[coderabbitai]

🧹 Nitpick comments (1)

cpp/tests/mr/aligned_mr_tests.cpp (1)

252-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding a test for the overflow failure case.

The three new tests cover the safe boundary and no-op cases well. There is no test for the case where overflow actually occurs (e.g., align_up(SIZE_MAX, 256)), which would trigger the new assert in debug builds. If death tests (EXPECT_DEATH) are acceptable in this codebase, adding one would close the coverage gap and directly validate the overflow guard.

🤖 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 `@cpp/tests/mr/aligned_mr_tests.cpp` around lines 252 - 282, Add a death test
in the AlignedTest suite covering
align_up(std::numeric_limits<std::size_t>::max(), 256) and assert that it
terminates due to the overflow guard. Follow the repository’s existing
death-test conventions, if available, and keep the current safe-boundary and
no-op tests unchanged.

Source: Learnings

🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@cpp/tests/mr/aligned_mr_tests.cpp`:
- Around line 252-282: Add a death test in the AlignedTest suite covering
align_up(std::numeric_limits<std::size_t>::max(), 256) and assert that it
terminates due to the overflow guard. Follow the repository’s existing
death-test conventions, if available, and keep the current safe-boundary and
no-op tests unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ebfd1397-1999-4d51-828b-a990ae9a898b

📥 Commits

Reviewing files that changed from the base of the PR and between 7c80d0f and 4589d01.

📒 Files selected for processing (3)

• cpp/include/rmm/aligned.hpp
• cpp/src/aligned.cpp
• cpp/tests/mr/aligned_mr_tests.cpp

[nethum529]

Pushed the revision.

• @harrism: align_up is back to noexcept so it stays callable from destructors. Invariants (power-of-2 alignment, non-overflow) are now enforced with assert in debug builds rather than throwing, so the release ABI and the destructor-safe contract are both preserved. Updated the doc comments and PR title/description to match.
• CodeRabbit overflow-failure coverage: added AlignedDeathTest.AlignUpOverflowAborts (guarded by #ifndef NDEBUG, matching the existing death-test convention in cuda_stream_tests.cpp) which asserts align_up(SIZE_MAX, 256) aborts on the overflow guard.

[nethum529]

Updated the branch against latest upstream main (including the new versioned ABI namespace from #2462, merged cleanly). Both review asks were addressed in the earlier push: the static_assert(noexcept(...)) is removed, and the vague happy-path test is replaced by purpose-named boundary tests plus a debug-only overflow death test. Built and ran the aligned tests locally after the merge.

[bdice]

Great. Thanks for reworking this.

[bdice]

/ok to test 0bf77ec