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

[Kim Barrett]

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

>> Casper Norrbin has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - removed align_up_or_min test from test_align
>>  - psoldgen check + removed align_up_or_min
>
> 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(static_cast<T>(small_value), alignment));
> +  EXPECT_TRUE(can_align_up(static_cast<T>(-small_value), alignment));
> +  EXPECT_TRU...

I forgot to add any description comments for can_align_up.  And align_up description should say can_align_up
is a precondition.

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

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