8316422: TestIntegerUnsignedDivMod.java triggers "invalid layout" assert in FrameValues::validate

Reviewed-by: thartmann, never

src/hotspot/share/c1/c1_GraphBuilder.cpp

@@ -2517,6 +2517,8 @@ XHandlers* GraphBuilder::handle_exception(Instruction* instruction) {
 
         // xhandler start with an empty expression stack
         if (cur_state->stack_size() != 0) {
+          // locals are preserved
+          // stack will be truncated
           cur_state = cur_state->copy(ValueStack::ExceptionState, cur_state->bci());
         }
         if (instruction->exception_state() == nullptr) {
@@ -2566,15 +2568,19 @@ XHandlers* GraphBuilder::handle_exception(Instruction* instruction) {
       // This scope and all callees do not handle exceptions, so the local
       // variables of this scope are not needed. However, the scope itself is
       // required for a correct exception stack trace -> clear out the locals.
-      if (_compilation->env()->should_retain_local_variables()) {
-        cur_state = cur_state->copy(ValueStack::ExceptionState, cur_state->bci());
-      } else {
-        cur_state = cur_state->copy(ValueStack::EmptyExceptionState, cur_state->bci());
-      }
+      // Stack and locals are invalidated but not truncated in caller state.
       if (prev_state != nullptr) {
+        assert(instruction->exception_state() != nullptr, "missed set?");
+        ValueStack::Kind exc_kind = ValueStack::empty_exception_kind(true /* caller */);
+        cur_state = cur_state->copy(exc_kind, cur_state->bci());
+        // reset caller exception state
         prev_state->set_caller_state(cur_state);
-      }
-      if (instruction->exception_state() == nullptr) {
+      } else {
+        assert(instruction->exception_state() == nullptr, "already set");
+        // set instruction exception state
+        // truncate stack
+        ValueStack::Kind exc_kind = ValueStack::empty_exception_kind();
+        cur_state = cur_state->copy(exc_kind, cur_state->bci());
         instruction->set_exception_state(cur_state);
       }
     }
@@ -3487,11 +3493,9 @@ ValueStack* GraphBuilder::copy_state_exhandling_with_bci(int bci) {
 ValueStack* GraphBuilder::copy_state_for_exception_with_bci(int bci) {
   ValueStack* s = copy_state_exhandling_with_bci(bci);
   if (s == nullptr) {
-    if (_compilation->env()->should_retain_local_variables()) {
-      s = state()->copy(ValueStack::ExceptionState, bci);
-    } else {
-      s = state()->copy(ValueStack::EmptyExceptionState, bci);
-    }
+    // no handler, no need to retain locals
+    ValueStack::Kind exc_kind = ValueStack::empty_exception_kind();
+    s = state()->copy(exc_kind, bci);
   }
   return s;
 }

src/hotspot/share/c1/c1_LIRGenerator.cpp

@@ -403,8 +403,20 @@ CodeEmitInfo* LIRGenerator::state_for(Instruction* x, ValueStack* state, bool ig
 
   ValueStack* s = state;
   for_each_state(s) {
-    if (s->kind() == ValueStack::EmptyExceptionState) {
-      assert(s->stack_size() == 0 && s->locals_size() == 0 && (s->locks_size() == 0 || s->locks_size() == 1), "state must be empty");
+    if (s->kind() == ValueStack::EmptyExceptionState ||
+        s->kind() == ValueStack::CallerEmptyExceptionState)
+    {
+#ifdef ASSERT
+      int index;
+      Value value;
+      for_each_stack_value(s, index, value) {
+        fatal("state must be empty");
+      }
+      for_each_local_value(s, index, value) {
+        fatal("state must be empty");
+      }
+#endif
+      assert(s->locks_size() == 0 || s->locks_size() == 1, "state must be empty");
       continue;
     }
 

src/hotspot/share/c1/c1_LinearScan.cpp

@@ -2921,16 +2921,10 @@ IRScopeDebugInfo* LinearScan::compute_debug_info_for_scope(int op_id, IRScope* c
 
       assert(locals->length() == pos, "must match");
     }
-    assert(locals->length() == cur_scope->method()->max_locals(), "wrong number of locals");
-    assert(locals->length() == cur_state->locals_size(), "wrong number of locals");
-  } else if (cur_scope->method()->max_locals() > 0) {
-    assert(cur_state->kind() == ValueStack::EmptyExceptionState, "should be");
-    nof_locals = cur_scope->method()->max_locals();
-    locals = new GrowableArray<ScopeValue*>(nof_locals);
-    for(int i = 0; i < nof_locals; i++) {
-      locals->append(_illegal_value);
-    }
+    assert(locals->length() == nof_locals, "wrong number of locals");
   }
+  assert(nof_locals == cur_scope->method()->max_locals(), "wrong number of locals");
+  assert(nof_locals == cur_state->locals_size(), "wrong number of locals");
 
   // describe expression stack
   int nof_stack = cur_state->stack_size();
@@ -2939,8 +2933,8 @@ IRScopeDebugInfo* LinearScan::compute_debug_info_for_scope(int op_id, IRScope* c
 
     int pos = 0;
     while (pos < nof_stack) {
-      Value expression = cur_state->stack_at_inc(pos);
-      append_scope_value(op_id, expression, expressions);
+      Value expression = cur_state->stack_at(pos);
+      pos += append_scope_value(op_id, expression, expressions);
 
       assert(expressions->length() == pos, "must match");
     }

src/hotspot/share/c1/c1_ValueStack.cpp

@@ -51,12 +51,32 @@ ValueStack::ValueStack(ValueStack* copy_from, Kind kind, int bci)
   , _stack(copy_from->stack_size_for_copy(kind))
   , _locks(copy_from->locks_size() == 0 ? nullptr : new Values(copy_from->locks_size()))
 {
-  assert(kind != EmptyExceptionState || !Compilation::current()->env()->should_retain_local_variables(), "need locals");
-  if (kind != EmptyExceptionState) {
+  switch (kind) {
+  case EmptyExceptionState:
+  case CallerEmptyExceptionState:
+    assert(!Compilation::current()->env()->should_retain_local_variables(), "need locals");
+    // set to all nulls, like clear_locals()
+    for (int i = 0; i < copy_from->locals_size(); ++i) {
+      _locals.append(nullptr);
+    }
+    break;
+  default:
     _locals.appendAll(&copy_from->_locals);
   }
 
-  if (kind != ExceptionState && kind != EmptyExceptionState) {
+  switch (kind) {
+  case ExceptionState:
+  case EmptyExceptionState:
+    assert(stack_size() == 0, "fix stack_size_for_copy");
+    break;
+  case CallerExceptionState:
+  case CallerEmptyExceptionState:
+    // set to all nulls
+    for (int i = 0; i < copy_from->stack_size(); ++i) {
+      _stack.append(nullptr);
+    }
+    break;
+  default:
     _stack.appendAll(&copy_from->_stack);
   }
 
@@ -68,10 +88,7 @@ ValueStack::ValueStack(ValueStack* copy_from, Kind kind, int bci)
 }
 
 int ValueStack::locals_size_for_copy(Kind kind) const {
-  if (kind != EmptyExceptionState) {
-    return locals_size();
-  }
-  return 0;
+  return locals_size();
 }
 
 int ValueStack::stack_size_for_copy(Kind kind) const {
@@ -221,10 +238,15 @@ void ValueStack::print() {
   } else {
     InstructionPrinter ip;
     for (int i = 0; i < stack_size();) {
+      tty->print("stack %d ", i);
       Value t = stack_at_inc(i);
-      tty->print("%2d  ", i);
-      tty->print("%c%d ", t->type()->tchar(), t->id());
-      ip.print_instr(t);
+      if (t == nullptr) {
+        tty->print("null");
+      } else {
+        tty->print("%2d  ", i);
+        tty->print("%c%d ", t->type()->tchar(), t->id());
+        ip.print_instr(t);
+      }
       tty->cr();
     }
   }
@@ -284,7 +306,9 @@ void ValueStack::verify() {
   int i;
   for (i = 0; i < stack_size(); i++) {
     Value v = _stack.at(i);
-    if (v == nullptr) {
+    if (kind() == empty_exception_kind(true /* caller */)) {
+      assert(v == nullptr, "should be empty");
+    } else if (v == nullptr) {
       assert(_stack.at(i - 1)->type()->is_double_word(), "only hi-words are null on stack");
     } else if (v->type()->is_double_word()) {
       assert(_stack.at(i + 1) == nullptr, "hi-word must be null");
@@ -293,7 +317,9 @@ void ValueStack::verify() {
 
   for (i = 0; i < locals_size(); i++) {
     Value v = _locals.at(i);
-    if (v != nullptr && v->type()->is_double_word()) {
+    if (kind() == EmptyExceptionState) {
+      assert(v == nullptr, "should be empty");
+    } else if (v != nullptr && v->type()->is_double_word()) {
       assert(_locals.at(i + 1) == nullptr, "hi-word must be null");
     }
   }

src/hotspot/share/c1/c1_ValueStack.hpp

@@ -34,8 +34,18 @@ class ValueStack: public CompilationResourceObj {
     CallerState,         // Caller state when inlining
     StateBefore,         // Before before execution of instruction
     StateAfter,          // After execution of instruction
-    ExceptionState,      // Exception handling of instruction
-    EmptyExceptionState, // Exception handling of instructions not covered by an xhandler
+    // Exception states for an instruction.
+    // Dead stack items or locals may be invalidated or cleared/removed.
+    // Locals are retained if needed for JVMTI.
+    // "empty" exception states are used when there is no handler,
+    // and invalidate the locals.
+    // "leaf" exception states clear the stack.
+    // "caller" exception states are used for the parent/caller,
+    // and invalidate the stack.
+    ExceptionState,      // Exception state for leaf with handler, stack cleared
+    EmptyExceptionState, // Exception state for leaf w/o handler, stack cleared, locals invalidated
+    CallerExceptionState, // Exception state for parent with handler, stack invalidated
+    CallerEmptyExceptionState, // Exception state for parent w/o handler, stack+locals invalidated
     BlockBeginState      // State of BlockBegin instruction with phi functions of this block
   };
 
@@ -75,10 +85,16 @@ class ValueStack: public CompilationResourceObj {
   ValueStack* copy(Kind new_kind, int new_bci)   { return new ValueStack(this, new_kind, new_bci); }
   ValueStack* copy_for_parsing()                 { return new ValueStack(this, Parsing, -99); }
 
+  // Used when no exception handler is found
+  static Kind empty_exception_kind(bool caller = false) {
+    return Compilation::current()->env()->should_retain_local_variables() ?
+      (caller ? CallerExceptionState : ExceptionState) : // retain locals
+      (caller ? CallerEmptyExceptionState : EmptyExceptionState);   // clear locals
+  }
+
   void set_caller_state(ValueStack* s)           {
-    assert(kind() == EmptyExceptionState ||
-           (Compilation::current()->env()->should_retain_local_variables() && kind() == ExceptionState),
-           "only EmptyExceptionStates can be modified");
+    assert(kind() == empty_exception_kind(false) || kind() == empty_exception_kind(true),
+           "only empty exception states can be modified");
     _caller_state = s;
   }
 
@@ -133,14 +149,14 @@ class ValueStack: public CompilationResourceObj {
   // stack access
   Value stack_at(int i) const {
     Value x = _stack.at(i);
-    assert(!x->type()->is_double_word() ||
+    assert(x == nullptr || !x->type()->is_double_word() ||
            _stack.at(i + 1) == nullptr, "hi-word of doubleword value must be null");
     return x;
   }
 
   Value stack_at_inc(int& i) const {
     Value x = stack_at(i);
-    i += x->type()->size();
+    i += ((x == nullptr) ? 1 : x->type()->size());
     return x;
   }
 
@@ -260,7 +276,8 @@ class ValueStack: public CompilationResourceObj {
   int temp_var = state->stack_size();                                                          \
   for (index = 0;                                                                              \
        index < temp_var && (value = state->stack_at(index), true);                             \
-       index += value->type()->size())
+       index += (value == nullptr ? 1 : value->type()->size()))                                \
+    if (value != nullptr)
 
 
 #define for_each_lock_value(state, index, value)                                               \

test/hotspot/jtreg/compiler/exceptions/TestDeoptExceptionState.java

@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+* @test
+* @bug 8316422
+* @summary Test exception state used for deoptimization.
+* @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+VerifyStack -XX:+DeoptimizeALot
+*      -Xcomp -XX:TieredStopAtLevel=1 -XX:CompileOnly=compiler.exceptions.TestDeoptExceptionState::test
+*      compiler.exceptions.TestDeoptExceptionState
+*/
+
+package compiler.exceptions;
+
+public class TestDeoptExceptionState {
+    private static int res = 0;
+
+    public static void main(String args[]) {
+        int x = 42;
+        int y = 1 + test();
+        System.out.println("Foo " + x + " " + y);
+    }
+
+    public static int test() {
+      int x = 42;
+      int y = 1 + test1();
+      return x + y;
+    }
+
+    public static int test1() {
+        for (int i = 0; i < 100; i++) {
+            try {
+                divZero();
+            } catch (ArithmeticException ea) {
+                // Expected
+            }
+        }
+        return 1;
+    }
+
+    public static void divZero() {
+        res += div(0, 0);
+    }
+
+    public static long div(long dividend, long divisor) {
+        return dividend / divisor;
+    }
+}