RFR: 8344026: [s390x] ubsan failure: signed integer overflow in c1_LIRGenerator_s390.cpp

[Martin Doerr]

On Fri, 15 Nov 2024 10:04:51 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

> This PR adds `c > 0 && c < max_jint` check in c1_LIRGenerator_s390.cpp. Please look JBS for more info.

Are more simple solution would be to use unsigned arithmetic which avoids UB: `is_power_of_2((juint)c + 1)`

> > Are more simple solution would be to use unsigned arithmetic which avoids UB: `is_power_of_2((juint)c + 1)`
> 
> On s390x then we have another issue:
> 
> ```c++
> if (tmp->is_valid()) {
>     if (is_power_of_2(c + 1)) {
>       __ move(left, tmp);
>       __ shift_left(left, log2i_exact(c + 1), left);
>       __ sub(left, tmp, result);
>       return true;
>     } else if (is_power_of_2(c - 1)) {
>       __ move(left, tmp);
>       __ shift_left(left, log2i_exact(c - 1), left);
>       __ add(left, tmp, result);
>       return true;
>     }
>   }
> ```
> 
> `__ sub(left, tmp, result);` will then contain `INT_MAX`+1 value.

Ok. That would need more adaptations. I'm also ok with `c > 0 && c < max_jint`.

The computation would still be correct with `is_power_of_2((juint)c + 1)` and `log2i_exact((juint)c + 1)`.
Integer shift left, add, sub and multiply are exactly the same operations regardless of signed or unsigned. (Except regarding flags which are not relevant here.)
The only thing we need to fix is UB. So, both solutions should work.

src/hotspot/share/c1/c1_LIRGenerator.cpp line 533:

> 531:         if (right->is_constant()) {
> 532:           jint c = right->as_jint();
> 533:           if (c > 0 && c < max_jint) {

This prevents platform specific optimizations for negative `c`. E.g. multiplication by -1 could be strength reduced to a negation.

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

PR Comment: https://git.openjdk.org/jdk/pull/22144#issuecomment-2478565533
PR Comment: https://git.openjdk.org/jdk/pull/22144#issuecomment-2478604647
PR Comment: https://git.openjdk.org/jdk/pull/22144#issuecomment-2478627490
PR Review Comment: https://git.openjdk.org/jdk/pull/22144#discussion_r1843594609