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

[Casper Norrbin]

On Tue, 4 Mar 2025 09:29:48 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> src/hotspot/share/utilities/align.hpp line 96:
>> 
>>> 94: 
>>> 95: template <typename T, typename A>
>>> 96: inline T* align_up_or_null(T* ptr, A alignment) {
>> 
>> Rather than specialized align_up variants, I think better and more generally
>> usable would be a predicate for testing whether align_up is safe.  Something
>> like a `bool can_align_up(value, alignment)` function.
>> 
>> I'm happy to see align_up_or_min removed from this PR for that reason, as well
>> as because it has usability issues.  Is a min-valued return indicating failure
>> to align?  Or was it just the argument is min-valued and already aligned?
>> Consider passing an unsigned zero value as the value to be aligned.
>> 
>> This seems to work:
>> 
>> diff --git a/src/hotspot/share/utilities/align.hpp b/src/hotspot/share/utilities/align.hpp
>> index b67e61036a0..d73e8e086ca 100644
>> --- a/src/hotspot/share/utilities/align.hpp
>> +++ b/src/hotspot/share/utilities/align.hpp
>> @@ -30,6 +30,8 @@
>>  #include "utilities/debug.hpp"
>>  #include "utilities/globalDefinitions.hpp"
>>  #include "utilities/powerOfTwo.hpp"
>> +
>> +#include <limits>
>>  #include <type_traits>
>>  
>>  // Compute mask to use for aligning to or testing alignment.
>> @@ -70,6 +72,17 @@ constexpr T align_down(T size, A alignment) {
>>    return result;
>>  }
>>  
>> +template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
>> +constexpr bool can_align_up(T size, A alignment) {
>> +  return align_down(std::numeric_limits<T>::max(), alignment) >= size;
>> +}
>> +
>> +template<typename A>
>> +inline bool can_align_up(const void* p, A alignment) {
>> +  static_assert(sizeof(p) == sizeof(uintptr_t), "assumption");
>> +  return can_align_up(reinterpret_cast<uintptr_t>(p), alignment);
>> +}
>> +
>>  template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
>>  constexpr T align_up(T size, A alignment) {
>>    T adjusted = checked_cast<T>(size + alignment_mask(alignment));
>> diff --git a/test/hotspot/gtest/utilities/test_align.cpp b/test/hotspot/gtest/utilities/test_align.cpp
>> index 3c03fd5f24d..2112d43feab 100644
>> --- a/test/hotspot/gtest/utilities/test_align.cpp
>> +++ b/test/hotspot/gtest/utilities/test_align.cpp
>> @@ -27,6 +27,51 @@
>>  #include "unittest.hpp"
>>  
>>  #include <limits>
>> +#include <type_traits>
>> +
>> +template<typename T, typename A>
>> +static constexpr void test_can_align_up() {
>> +  int alignment_value = 4;
>> +  int small_value = 63;
>> +  A alignment = static_cast<A>(alignment_value);
>> +
>> +  EXPECT_TRUE(can_align_up...
>
> I forgot to add any description comments for can_align_up.  And align_up description should say can_align_up
> is a precondition.

I removed `align_up_or_null` in favour of `can_align_up`, and I agree that it feels more usable.

Instead of `align_up_or_null`, we can now either return early or set a different value if `can_align_up` fails. I updated the files here and to me it feels like an improvement.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23711#discussion_r1979762563