8336042: Caller/callee param size mismatch in deoptimization causes crash

Co-authored-by: Richard Reingruber <[email protected]>
Reviewed-by: pchilanomate, rrich, vlivanov, never

Author: dean-long

src/hotspot/cpu/aarch64/abstractInterpreter_aarch64.cpp

@@ -149,7 +149,8 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    assert(locals <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(locals >= interpreter_frame->sender_sp() + max_locals - 1, "bad placement");
   }
 #endif
 

src/hotspot/cpu/arm/abstractInterpreter_arm.cpp

@@ -131,6 +131,15 @@ void AbstractInterpreter::layout_activation(Method* method,
 
   intptr_t* locals = interpreter_frame->sender_sp() + max_locals - 1;
 
+#ifdef ASSERT
+  if (caller->is_interpreted_frame()) {
+    // Test exact placement on top of caller args
+    intptr_t* l2 = caller->interpreter_frame_last_sp() + caller_actual_parameters - 1;
+    assert(l2 <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(l2 >= locals, "bad placement");
+  }
+#endif
+
   interpreter_frame->interpreter_frame_set_locals(locals);
   BasicObjectLock* montop = interpreter_frame->interpreter_frame_monitor_begin();
   BasicObjectLock* monbot = montop - moncount;

src/hotspot/cpu/ppc/abstractInterpreter_ppc.cpp

@@ -128,6 +128,15 @@ void AbstractInterpreter::layout_activation(Method* method,
     caller->interpreter_frame_esp() + caller_actual_parameters :
     caller->sp() + method->max_locals() - 1 + (frame::java_abi_size / Interpreter::stackElementSize);
 
+#ifdef ASSERT
+  if (caller->is_interpreted_frame()) {
+    assert(locals_base <= caller->interpreter_frame_expression_stack(), "bad placement");
+    const int caller_abi_bytesize = (is_bottom_frame ? frame::top_ijava_frame_abi_size : frame::parent_ijava_frame_abi_size);
+    intptr_t* l2 = caller->sp() + method->max_locals() - 1 + (caller_abi_bytesize / Interpreter::stackElementSize);
+    assert(locals_base >= l2, "bad placement");
+  }
+#endif
+
   intptr_t* monitor_base = caller->sp() - frame::ijava_state_size / Interpreter::stackElementSize;
   intptr_t* monitor      = monitor_base - (moncount * frame::interpreter_frame_monitor_size());
   intptr_t* esp_base     = monitor - 1;

src/hotspot/cpu/riscv/abstractInterpreter_riscv.cpp

@@ -141,7 +141,8 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    assert(locals <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(locals >= interpreter_frame->sender_sp() + max_locals - 1, "bad placement");
   }
 #endif
 

src/hotspot/cpu/s390/abstractInterpreter_s390.cpp

@@ -182,6 +182,13 @@ void AbstractInterpreter::layout_activation(Method* method,
   intptr_t* sender_sp;
   if (caller->is_interpreted_frame()) {
     sender_sp = caller->interpreter_frame_top_frame_sp();
+#ifdef ASSERT
+    assert(locals_base <= caller->interpreter_frame_expression_stack(), "bad placement");
+    // Test caller-aligned placement vs callee-aligned
+    intptr_t* l2 = (caller->sp() + method->max_locals() - 1 +
+      frame::z_parent_ijava_frame_abi_size / Interpreter::stackElementSize);
+    assert(locals_base >= l2, "bad placement");
+#endif
   } else if (caller->is_compiled_frame()) {
     sender_sp = caller->fp() - caller->cb()->frame_size();
     // The bottom frame's sender_sp is its caller's unextended_sp.

src/hotspot/cpu/x86/abstractInterpreter_x86.cpp

@@ -87,7 +87,10 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    // Test exact placement on top of caller args
+    intptr_t* l2 = caller->interpreter_frame_last_sp() + caller_actual_parameters - 1;
+    assert(l2 <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(l2 >= locals, "bad placement");
   }
 #endif
 

src/hotspot/share/interpreter/bytecode.hpp

@@ -226,6 +226,8 @@ class Bytecode_invoke: public Bytecode_member_ref {
 
   bool has_appendix();
 
+  bool has_member_arg() const;
+
   int size_of_parameters() const;
 
  private:

src/hotspot/share/interpreter/bytecode.inline.hpp

@@ -28,6 +28,7 @@
 #include "interpreter/bytecode.hpp"
 
 #include "oops/cpCache.inline.hpp"
+#include "prims/methodHandles.hpp"
 
 inline bool Bytecode_invoke::has_appendix() {
   if (invoke_code() == Bytecodes::_invokedynamic) {
@@ -37,4 +38,13 @@ inline bool Bytecode_invoke::has_appendix() {
   }
 }
 
+inline bool Bytecode_invoke::has_member_arg() const {
+  // NOTE: We could resolve the call and use the resolved adapter method here, but this function
+  // is used by deoptimization, where resolving could lead to problems, so we avoid that here
+  // by doing things symbolically.
+  //
+  // invokedynamic instructions don't have a class but obviously don't have a MemberName appendix.
+  return !is_invokedynamic() && MethodHandles::has_member_arg(klass(), name());
+}
+
 #endif // SHARE_INTERPRETER_BYTECODE_INLINE_HPP

src/hotspot/share/runtime/deoptimization.cpp

@@ -36,6 +36,7 @@
 #include "gc/shared/collectedHeap.hpp"
 #include "gc/shared/memAllocator.hpp"
 #include "interpreter/bytecode.hpp"
+#include "interpreter/bytecode.inline.hpp"
 #include "interpreter/bytecodeStream.hpp"
 #include "interpreter/interpreter.hpp"
 #include "interpreter/oopMapCache.hpp"
@@ -641,11 +642,12 @@ Deoptimization::UnrollBlock* Deoptimization::fetch_unroll_info_helper(JavaThread
   bool caller_was_method_handle = false;
   if (deopt_sender.is_interpreted_frame()) {
     methodHandle method(current, deopt_sender.interpreter_frame_method());
-    Bytecode_invoke cur = Bytecode_invoke_check(method, deopt_sender.interpreter_frame_bci());
-    if (cur.is_invokedynamic() || cur.is_invokehandle()) {
-      // Method handle invokes may involve fairly arbitrary chains of
-      // calls so it's impossible to know how much actual space the
-      // caller has for locals.
+    Bytecode_invoke cur(method, deopt_sender.interpreter_frame_bci());
+    if (cur.has_member_arg()) {
+      // This should cover all real-world cases.  One exception is a pathological chain of
+      // MH.linkToXXX() linker calls, which only trusted code could do anyway.  To handle that case, we
+      // would need to get the size from the resolved method entry.  Another exception would
+      // be an invokedynamic with an adapter that is really a MethodHandle linker.
       caller_was_method_handle = true;
     }
   }
@@ -748,9 +750,14 @@ Deoptimization::UnrollBlock* Deoptimization::fetch_unroll_info_helper(JavaThread
   }
 #endif
 
+  int caller_actual_parameters = -1; // value not used except for interpreted frames, see below
+  if (deopt_sender.is_interpreted_frame()) {
+    caller_actual_parameters = callee_parameters + (caller_was_method_handle ? 1 : 0);
+  }
+
   UnrollBlock* info = new UnrollBlock(array->frame_size() * BytesPerWord,
                                       caller_adjustment * BytesPerWord,
-                                      caller_was_method_handle ? 0 : callee_parameters,
+                                      caller_actual_parameters,
                                       number_of_frames,
                                       frame_sizes,
                                       frame_pcs,
@@ -939,7 +946,7 @@ JRT_LEAF(BasicType, Deoptimization::unpack_frames(JavaThread* thread, int exec_m
       if (Bytecodes::is_invoke(cur_code)) {
         Bytecode_invoke invoke(mh, iframe->interpreter_frame_bci());
         cur_invoke_parameter_size = invoke.size_of_parameters();
-        if (i != 0 && !invoke.is_invokedynamic() && MethodHandles::has_member_arg(invoke.klass(), invoke.name())) {
+        if (i != 0 && invoke.has_member_arg()) {
           callee_size_of_parameters++;
         }
       }

src/hotspot/share/runtime/vframeArray.cpp

@@ -25,6 +25,7 @@
 #include "classfile/vmSymbols.hpp"
 #include "code/vmreg.inline.hpp"
 #include "interpreter/bytecode.hpp"
+#include "interpreter/bytecode.inline.hpp"
 #include "interpreter/interpreter.hpp"
 #include "memory/allocation.inline.hpp"
 #include "memory/resourceArea.hpp"
@@ -610,10 +611,7 @@ void vframeArray::unpack_to_stack(frame &unpack_frame, int exec_mode, int caller
       methodHandle caller(current, elem->method());
       methodHandle callee(current, element(index - 1)->method());
       Bytecode_invoke inv(caller, elem->bci());
-      // invokedynamic instructions don't have a class but obviously don't have a MemberName appendix.
-      // NOTE:  Use machinery here that avoids resolving of any kind.
-      const bool has_member_arg =
-          !inv.is_invokedynamic() && MethodHandles::has_member_arg(inv.klass(), inv.name());
+      const bool has_member_arg = inv.has_member_arg();
       callee_parameters = callee->size_of_parameters() + (has_member_arg ? 1 : 0);
       callee_locals     = callee->max_locals();
     }

test/hotspot/jtreg/compiler/jsr292/MHDeoptTest.java

@@ -0,0 +1,97 @@
+/*
+ * Copyright (c) 2025, 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.
+ */
+
+package compiler.jsr292;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodHandles.Lookup;
+import java.lang.invoke.MethodType;
+import java.lang.reflect.Method;
+
+/*
+ * @test
+ * @bug 8336042
+ * @library /test/lib /
+ *
+ * @run main/bootclasspath/othervm -Xbatch -XX:-TieredCompilation compiler.jsr292.MHDeoptTest
+ *
+ */
+public class MHDeoptTest {
+
+    static int xx = 0;
+
+    public static void main(String[] args) throws Throwable {
+        MethodHandle mh1 = MethodHandles.lookup().findStatic(MHDeoptTest.class, "body1", MethodType.methodType(int.class));
+        MethodHandle mh2 = MethodHandles.lookup().findStatic(MHDeoptTest.class, "body2", MethodType.methodType(int.class));
+        MethodHandle[] arr = new MethodHandle[] {mh2, mh1};
+
+        for (MethodHandle mh : arr) {
+            for (int i = 1; i < 50_000; i++) {
+                xx = i;
+                mainLink(mh);
+            }
+        }
+
+    }
+
+    static int mainLink(MethodHandle mh) throws Throwable {
+        return (int)mh.invokeExact();
+    }
+
+    static int cnt = 1000;
+
+    static int body1() {
+        int limit = 0x7fff;
+        // uncommon trap
+        if (xx == limit) {
+            // OSR
+            for (int i = 0; i < 50_000; i++) {
+            }
+            ++cnt;
+            ++xx;
+        }
+        if (xx == limit + 1) {
+            return cnt + 1;
+        }
+        return cnt;
+    }
+
+    static int body2() {
+        int limit = 0x7fff;
+        int dummy = 0;
+        // uncommon trap
+        if (xx == limit) {
+            // OSR
+            for (int i = 0; i < 50_000; i++) {
+            }
+            ++cnt;
+            ++xx;
+        }
+        if (xx == limit + 1) {
+            return cnt + 1;
+        }
+        return cnt;
+    }
+
+}