RFR: 8318127: align_up has potential overflow [v4]

[Kim Barrett]

On Wed, 9 Oct 2024 12:37:40 GMT, Casper Norrbin <cnorrbin at openjdk.org> wrote:

>> Hi everyone,
>> 
>> The `align_up` function contained code which could potentially overflow and produce an incorrect result. This PR adds an assert to check for such.
>> 
>> Additionally, two test case that previously caused an overflow have been updated to use the highest possible values that do not trigger an overflow.
>
> Casper Norrbin has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - update copyright header
>  - align_up death tests

Changes requested by kbarrett (Reviewer).

test/hotspot/gtest/utilities/test_align.cpp line 164:

> 162: static void test_fail_alignment() {
> 163:   A alignment = max_alignment<A>();
> 164:   T value = std::numeric_limits<T>::max() - checked_cast<T>(alignment) + 10;

I think a simpler expression here would be

T value = align_down(std::numeric_limits<T>::max(), alignment) + 1;

I found it a bit harder to convince myself the original was correct.

test/hotspot/gtest/utilities/test_align.cpp line 165:

> 163:   A alignment = max_alignment<A>();
> 164:   T value = std::numeric_limits<T>::max() - checked_cast<T>(alignment) + 10;
> 165:   // Aligning up would overflow, as there isnt enough room for alignment

s/isnt/isn't/

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

PR Review: https://git.openjdk.org/jdk/pull/20808#pullrequestreview-2357186705
PR Review Comment: https://git.openjdk.org/jdk/pull/20808#discussion_r1793555574
PR Review Comment: https://git.openjdk.org/jdk/pull/20808#discussion_r1793542078