RFR: 8333177: Invalid value used for enum Cell in ciTypeFlow::get_start_state

[Christian Hagedorn]

On Mon, 3 Jun 2024 11:57:58 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> Ubsan detected undefined behavior in `ciTypeFlow::get_start_state` because an invalid value of `4294967295` is assigned to enum `Cell`:
> https://github.com/openjdk/jdk/blob/ac7119f0d5319a3fb44dc67a938c3e1eb21b9202/src/hotspot/share/ci/ciTypeFlow.hpp#L150-L152
> 
> The problem is that if the C++ compiler decides to encode `Cell` with an unsigned int, casting a negative integer value will lead to an underflow and therefore a value > `Cell_max = INT_MAX`. Here, `state->tos()` returns a value < 0:
> https://github.com/openjdk/jdk/blob/ac7119f0d5319a3fb44dc67a938c3e1eb21b9202/src/hotspot/share/ci/ciTypeFlow.cpp#L407
> 
> which is casted to a `Cell`:
> https://github.com/openjdk/jdk/blob/ac7119f0d5319a3fb44dc67a938c3e1eb21b9202/src/hotspot/share/ci/ciTypeFlow.hpp#L211
> 
> I simply re-wrote the code to not require a negative `Cell` value to iterate over the locals and setting them to bottom type.
> 
> Thanks,
> Tobias

src/hotspot/share/ci/ciTypeFlow.cpp line 408:

> 406:   // Set the rest of the locals to bottom.
> 407:   while (state->stack_size() != 0) {
> 408:     state->push(state->bottom_type());

It might be more clear if the condition is `< 0` since we only enter the loop if the stack size is negative. Maybe we could also assert that the stack size is `<= 0` before the loop? Otherwise, nice cleanup!

Suggestion:

  assert(state->stack_size() <= 0, "stack size should not be strictly positive");
  while (state->stack_size() < 0) {
    state->push(state->bottom_type());

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19520#discussion_r1625724417