RFR: 8318127: align_up has potential overflow [v2]

[Casper Norrbin]

On Tue, 1 Oct 2024 14:19:48 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Casper Norrbin has refreshed the contents of this pull request, and previous commits have been removed. Incremental views are not available.
>
> 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.

The last change, adding `align_up_checked` has been reverted. I agree that for the uses I looked at, they all assumed the aligned value to be greater or equal to the original value.

Additionally, the only tests that failed were gtests that aligned to maximum values, something that is not representative of actual use.

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

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