RFR: 8318127: align_up has potential overflow [v8]

[Kim Barrett]

On Mon, 25 Nov 2024 23:31:38 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Casper Norrbin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   newline at eof
>
> src/hotspot/share/utilities/align.hpp line 75:
> 
>> 73: template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
>> 74: constexpr T align_up(T size, A alignment) {
>> 75:   T mask = checked_cast<T>(alignment_mask(alignment));
> 
> Should we assert that mask >= 0 or trust that the result of alignment_mask() is never negative?

alignment_mask requires the alignment to be a power of 2, so can't return a
negative value.

But now I'm wondering why the size and alignment are allowed to be of
different types in all these functions.  If alignment doesn't fit in T then it
seems like lots of things can go wrong.  Maybe alignment_mask should take the
size type as an additional (non-deduced) template parameter and return a value
of that type after checking it fits?  Or something like that, as that isn't
quite right for is_aligned.  All uses of alignment_mask do some implicit or
explicit conversion involving the size type.

Sounds like there's some followup work to do.  I'll file a JBS issue.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20808#discussion_r1857533533