RFR: 8318127: align_up has potential overflow [v2]

[Kim Barrett]

On Tue, 1 Oct 2024 12:59:12 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 one additional commit since the last revision:
> 
>   checked/unchecked align functions

Changes requested by kbarrett (Reviewer).

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

> 78: 
> 79: template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
> 80: constexpr T align_up_checked(T size, A alignment) {

I think this new version, adding align_up_checked, should not be done.

As I mentioned earlier, every use I've looked at expects the result to be not
less than the value being aligned.  That would mean all (or very nearly so)
uses would need to be changed to align_up_checked.  That's just backward.

Part of the problem the JBS issue wants to have addressed is that in the case of
signed value or pointer overflow, that actually is UB.  We want to try to catch
those.

If a modular variant is shown to be needed (and someone needs to find an actual
use case for it, and maybe a good name), then it can be added at that time. I
think such a variant should be restricted to unsigned integral values.

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

PR Review: https://git.openjdk.org/jdk/pull/20808#pullrequestreview-2340502567
PR Review Comment: https://git.openjdk.org/jdk/pull/20808#discussion_r1782956878