RFR: 8346916: [REDO] align_up has potential overflow [v9]

[Albert Mingkun Yang]

On Wed, 5 Mar 2025 21:26:08 GMT, Casper Norrbin <cnorrbin at openjdk.org> wrote:

>> Hi everyone,
>> 
>> The `align_up` function can potentially overflow, resulting in undefined behavior. Most use cases rely on the assumption that aligned_result >= original. To address this, I've added an assertion to verify this condition.
>> 
>> The original PR (#20808) missed cases where overflow checks already existed, so I've now went through usages of `align_up` and found the places with explicit checks. Most notably, #23168 added `align_up_or_null` to metaspace, but this function is also useful elsewhere. Given this, I relocated it to `align.hpp`, alongside the rest of the alignment functions.
>
> Casper Norrbin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed template paramter and moved ptr can_align_up

> Do we really want to allow passing nullptr to can_align_up(void* ptr, A alignment)?

I don't see any problem with allowing or passing `nullptr` to `can_align_up`, `align_up`, or `align_down`. The result should be `true` and have no effect, as if the argument were the integer `0`, right? Disallowing `nullptr` would introduce extra code in these functions, which would clutter the flow, in my opinion.

src/hotspot/share/utilities/align.hpp line 83:

> 81: constexpr T align_up(T size, A alignment) {
> 82:   T mask = checked_cast<T>(alignment_mask(alignment));
> 83:   assert(size <= std::numeric_limits<T>::max() - mask, "overflow");

Just curious, if `can_align_up` is precondition, why not `assert(can_align_up(...), "precondition")`? Then, the comment can even be dropped.

-------------

PR Review: https://git.openjdk.org/jdk/pull/23711#pullrequestreview-2673529828
PR Review Comment: https://git.openjdk.org/jdk/pull/23711#discussion_r1988763609