RFR: 8352140: UBSAN: fix the left shift of negative value in klass.hpp, array_layout_helper() [v3]

[Afshin Zafari]

On Sun, 8 Jun 2025 18:16:04 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Changes requested by kbarrett (Reviewer).
>
>> I made the `int->uint32` change at the minimum level, because the layout_helper constants are `public` in the `Kalss` class and are used all around Hotspot code, particularly in architecture dependent codes. Changing them to unsigned breaks the build (at SYMBOL generation phase). In addition, the two most-significant bits of the layout are used for `array` types and `lh < 0` (or more precisely `lh < _lh_neutral_value`) is used in C++ and assembly language of arch-dep code. @kimbarrett , can we limit the change here to `array_layout_helper` or should we proceed to use `uint32_t` instead?
> 
> I'm inclined against uglifying the code for a ubsan issue just to keep the
> change small. I'd rather we did the "right" fix instead, even though there's
> some fannout. I don't know how urgent we consider fixing the ubsan issue
> though.
> 
> But if we're going to do a minimal change, I like the @dean-long suggestion here:
> https://github.com/openjdk/jdk/pull/24184/files#r2090835514
> 
> The latest proposal also doesn't do anything about the issue I mentioned at
> the end of this comment:
> https://github.com/openjdk/jdk/pull/24184#discussion_r2055739014
> where there's a related problem with layout_helper_boolean_diffbit.
> That could be treated as a separate bug, in which case just file it.

> > I made the `int->uint32` change at the minimum level, because the layout_helper constants are `public` in the `Kalss` class and are used all around Hotspot code, particularly in architecture dependent codes. Changing them to unsigned breaks the build (at SYMBOL generation phase). In addition, the two most-significant bits of the layout are used for `array` types and `lh < 0` (or more precisely `lh < _lh_neutral_value`) is used in C++ and assembly language of arch-dep code. @kimbarrett , can we limit the change here to `array_layout_helper` or should we proceed to use `uint32_t` instead?
> 
> I'm inclined against uglifying the code for a ubsan issue just to keep the change small. I'd rather we did the "right" fix instead, even though there's some fannout. I don't know how urgent we consider fixing the ubsan issue though.
> 
> But if we're going to do a minimal change, I like the @dean-long suggestion here: https://github.com/openjdk/jdk/pull/24184/files#r2090835514
> 
> The latest proposal also doesn't do anything about the issue I mentioned at the end of this comment: [#24184 (comment)](https://github.com/openjdk/jdk/pull/24184#discussion_r2055739014) where there's a related problem with layout_helper_boolean_diffbit. That could be treated as a separate bug, in which case just file it.


Thanks for your input.
OK. I also agree. Then since the required changes are not in my expertise, I withdraw this PR and let qualified developers fix the issue.

For the bogus code in `layout_helper_boolean_diffbit`, [this](https://bugs.openjdk.org/browse/JDK-8358957)  is filed.

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

PR Comment: https://git.openjdk.org/jdk/pull/24184#issuecomment-2955195732