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

[Amit Kumar]

On Fri, 15 Nov 2024 11:01:12 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

> 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: 

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.

> 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.

I didn't get it. How this will affect -1 case ? 

I see that this is implementation for `strength_reduce_multiply`: 

bool LIRGenerator::strength_reduce_multiply(LIR_Opr left, jint c, LIR_Opr result, LIR_Opr tmp) {
  assert(left != result, "should be different registers");
  if (is_power_of_2(c + 1)) {
    __ shift_left(left, log2i_exact(c + 1), result);
    __ sub(result, left, result);
    return true;
  } else if (is_power_of_2(c - 1)) {
    __ shift_left(left, log2i_exact(c - 1), result);
    __ add(result, left, result);
    return true;
  }
  return false;
}

which will return false in case of `-1`.  

Basically even without my change, `c = -1` will set `did_strength_reduce` to `false`.

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

PR Comment: https://git.openjdk.org/jdk/pull/22144#issuecomment-2478569235
PR Review Comment: https://git.openjdk.org/jdk/pull/22144#discussion_r1843606825