Merge pull request #18474 from ThanHenderson/14990

Properly handle resolve-time invokeDynamic errors for OJDK MHs

Author: babsingh

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java

@@ -245,7 +245,7 @@ private static final Object resolveInvokeDynamic(long j9class, String name, Stri
 /*[ELSE] JAVA_SPEC_VERSION >= 11*/
 		try {
 			type = MethodTypeHelper.vmResolveFromMethodDescriptorString(methodDescriptor, access.getClassloader(classObject), null);
-/*[ENDIF] JAVA_SPEC_VERSION >= 11 */
+/*[ENDIF] JAVA_SPEC_VERSION >= 11*/
 
 			int bsmIndex = UNSAFE.getShort(bsmData);
 			int bsmArgCount = UNSAFE.getShort(bsmData + BSM_ARGUMENT_COUNT_OFFSET);
@@ -265,13 +265,10 @@ private static final Object resolveInvokeDynamic(long j9class, String name, Stri
 
 			Object[] appendixResult = new Object[1];
 
-			/* result[0] stores a MemberName object, which specifies the caller method (generated bytecodes), or a Throwable.
-			*
-			* This leads to a type check in the interpreter for the object stored in result[0].
-			*
-			* TODO: Investigate if the Throwable can be wrapped in a MemberName. This will help prevent type checks in the
-			* interpreter since result[0] will always be a MemberName. This will improve performance.
-			*/
+/*[IF JAVA_SPEC_VERSION >= 11]*/
+		try {
+/*[ENDIF] JAVA_SPEC_VERSION >= 11*/
+			/* result[0] stores a MemberName object, which specifies the caller method (generated bytecodes). */
 			result[0] = MethodHandleNatives.linkCallSite(classObject,
 					/* The second parameter is not used in Java 8 and Java 18+ (JDK bug: 8272614). */
 					/*[IF (JAVA_SPEC_VERSION > 8) & (JAVA_SPEC_VERSION < 18)]*/
@@ -281,23 +278,39 @@ private static final Object resolveInvokeDynamic(long j9class, String name, Stri
 
 			/* result[1] stores a MethodHandle object, which is used as the last argument to the caller method. */
 			result[1] = appendixResult[0];
-/*[IF JAVA_SPEC_VERSION < 11]*/
 		} catch (Throwable e) {
-			if (type == null) {
-				throw new BootstrapMethodError(e);
-			}
-
-			/* linkCallSite may correctly throw a BootstrapMethodError. */
+/*[IF JAVA_SPEC_VERSION < 11]*/
+			/* Before Java 11, if linkCallSite throws a BootstrapMethodError due to the CallSite binding
+			 * resolving to null, throw it at resolution time. This ensures that resolution will be
+			 * reattempted on the subsequent visit to the invokedynamic instruction in the interpreter.
+			 */
 			if (e instanceof BootstrapMethodError) {
 				throw e;
 			}
-
-			/* Any other throwables are wrapped in an invoke-time BootstrapMethodError exception throw. */
+/*[ENDIF] JAVA_SPEC_VERSION < 11*/
+			/* Any throwables are wrapped in an invoke-time BootstrapMethodError exception throw. */
 			try {
+				MethodHandle resultHandle;
 				MethodHandle thrower = MethodHandles.throwException(type.returnType(), BootstrapMethodError.class);
 				MethodHandle constructor = IMPL_LOOKUP.findConstructor(BootstrapMethodError.class, MethodType.methodType(void.class, Throwable.class));
-				MethodHandle resultHandle = MethodHandles.foldArguments(thrower, constructor.bindTo(e));
+
+				MethodHandle combiner = null;
+				if (e instanceof BootstrapMethodError) {
+					Throwable cause = e.getCause();
+					if (cause == null) {
+						combiner = constructor.bindTo(e.getMessage());
+					} else {
+						combiner = constructor.bindTo(cause);
+					}
+				} else {
+					combiner = constructor.bindTo(e);
+				}
+				resultHandle = MethodHandles.foldArguments(thrower, combiner);
+/*[IF JAVA_SPEC_VERSION >= 11]*/
+				MemberName memberName = resultHandle.internalForm().vmentry;
+/*[ELSE] JAVA_SPEC_VERSION >= 11*/
 				MemberName memberName = resultHandle.internalForm().compileToBytecode();
+/*[ENDIF] JAVA_SPEC_VERSION >= 11*/
 				result[0] = memberName;
 				result[1] = resultHandle;
 			} catch (IllegalAccessException iae) {
@@ -306,7 +319,6 @@ private static final Object resolveInvokeDynamic(long j9class, String name, Stri
 				throw new Error(nsme);
 			}
 		}
-/*[ENDIF] JAVA_SPEC_VERSION < 11 */
 		return (Object)result;
 /*[ELSE] OPENJDK_METHODHANDLES*/	
 		MethodHandle result = null;
@@ -332,7 +344,7 @@ private static final Object resolveInvokeDynamic(long j9class, String name, Stri
 			int bsmArgCount = UNSAFE.getShort(bsmData + BSM_ARGUMENT_COUNT_OFFSET);
 			long bsmArgs = bsmData + BSM_ARGUMENTS_OFFSET;
 			MethodHandle bsm = getCPMethodHandleAt(internalConstantPool, bsmIndex);
-			if (null == bsm) {
+			if (bsm == null) {
 				/*[MSG "K05cd", "unable to resolve 'bootstrap_method_ref' in '{0}' at index {1}"]*/
 				throw new NullPointerException(Msg.getString("K05cd", classObject.toString(), bsmIndex)); //$NON-NLS-1$
 			}

runtime/vm/BytecodeInterpreter.hpp

@@ -8865,29 +8865,23 @@ class INTERPRETER_CLASS
 		j9object_t invokeCacheArray = (ramConstantPool->ramClass->callSites)[index];
 
 		if (J9_EXPECTED(NULL != invokeCacheArray)) {
-			J9Class *clazz = J9OBJECT_CLAZZ(_currentThread, invokeCacheArray);
-			if (J9CLASS_IS_ARRAY(clazz)) {
-				/* Fetch target method and appendix from invokeCacheArray (2 element array)
-				 * Stack transitions from:
-				 * 		args <- SP
-				 * 		MH
-				 * To:
-				 * 		invokeCacheArray[1] "appendix" <- SP
-				 * 		args
-				 * 		MH
-				 *
-				 * and sendMethod is ((J9Method *)((j.l.MemberName)invokeCacheArray[0]) + vmtargetOffset)
-				 */
-				j9object_t memberName = (j9object_t)J9JAVAARRAYOFOBJECT_LOAD(_currentThread, invokeCacheArray, 0);
-				_sendMethod = (J9Method *)(UDATA)J9OBJECT_U64_LOAD(_currentThread, memberName, _vm->vmtargetOffset);
+			/* Fetch target method and appendix from invokeCacheArray (2 element array)
+			 * Stack transitions from:
+			 * 		args <- SP
+			 * 		MH
+			 * To:
+			 * 		invokeCacheArray[1] "appendix" <- SP
+			 * 		args
+			 * 		MH
+			 *
+			 * and sendMethod is ((J9Method *)((j.l.MemberName)invokeCacheArray[0]) + vmtargetOffset)
+			 */
+			j9object_t memberName = (j9object_t)J9JAVAARRAYOFOBJECT_LOAD(_currentThread, invokeCacheArray, 0);
+			_sendMethod = (J9Method *)(UDATA)J9OBJECT_U64_LOAD(_currentThread, memberName, _vm->vmtargetOffset);
 
-				j9object_t appendix = (j9object_t)J9JAVAARRAYOFOBJECT_LOAD(_currentThread, invokeCacheArray, 1);
-				if (NULL != appendix) {
-					*--_sp = (UDATA)appendix;
-				}
-			} else {
-				VM_VMHelpers::setExceptionPending(_currentThread, invokeCacheArray);
-				rc = GOTO_THROW_CURRENT_EXCEPTION;
+			j9object_t appendix = (j9object_t)J9JAVAARRAYOFOBJECT_LOAD(_currentThread, invokeCacheArray, 1);
+			if (NULL != appendix) {
+				*--_sp = (UDATA)appendix;
 			}
 		} else {
 			buildGenericSpecialStackFrame(REGISTER_ARGS, 0);

runtime/vm/resolvesupport.cpp

@@ -2239,24 +2239,23 @@ resolveInvokeDynamic(J9VMThread *vmThread, J9ConstantPool *ramCP, UDATA callSite
 		/* Check if an exception is already pending */
 		if (vmThread->currentException != NULL) {
 			/* Already a pending exception */
-			result = vmThread->currentException;
+			result = NULL;
 		} else if (NULL == result) {
 			setCurrentExceptionUTF(vmThread, J9VMCONSTANTPOOL_JAVALANGNULLPOINTEREXCEPTION, NULL);
-			result = vmThread->currentException;
-		}
-
-		/* The result can be an array or exception. Ensure that the result and its elements are
-		 * written/published before the result reference is stored.
-		 */
-		VM_AtomicSupport::writeBarrier();
-
-		J9MemoryManagerFunctions *gcFuncs = vmThread->javaVM->memoryManagerFunctions;
-		J9Class *j9class = J9_CLASS_FROM_CP(ramCP);
-		if (0 == gcFuncs->j9gc_objaccess_staticCompareAndSwapObject(vmThread, j9class, callSite, NULL, result)) {
-			/* Another thread beat this thread to updating the call site, ensure both threads
-			 * return the same method handle.
+		} else {
+			/* The result is an array. Ensure that the result and its elements are
+			 * written/published before the result reference is stored.
 			 */
-			result = *callSite;
+			VM_AtomicSupport::writeBarrier();
+
+			J9MemoryManagerFunctions *gcFuncs = vmThread->javaVM->memoryManagerFunctions;
+			J9Class *j9class = J9_CLASS_FROM_CP(ramCP);
+			if (0 == gcFuncs->j9gc_objaccess_staticCompareAndSwapObject(vmThread, j9class, callSite, NULL, result)) {
+				/* Another thread beat this thread to updating the call site, ensure both threads
+				 * return the same method handle.
+				 */
+				result = *callSite;
+			}
 		}
 #else /* defined(J9VM_OPT_OPENJDK_METHODHANDLE) */
 		/* Check if an exception is already pending */

test/functional/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java

@@ -373,30 +373,33 @@ public void test_boostrap_return_constant_MethodType() {
 		AssertJUnit.assertTrue(expected == mt);
 	}
 
-	// test that if resolved CallSite is null, the same error is rethrown
+	// Test to verify behaviour if a CallSite initially resolves to null
 	@Test(groups = { "level.extended" })
-	public void test_CallSiteNullErrorRethrown () {
+	public void test_CallSiteNullErrorRethrown() {
 		/* The bootstrap method associated with the indy call in test_CallSiteNullErrorRethrown
 		 * will return null the first time its called, and a valid CallSite for all repeat calls.
 		 */
 
-		// Java 8: NullPointerException is expected on the first run
-		// Java 11: BootstrapMethodError is expected on the first run
 		try {
 			com.ibm.j9.jsr292.indyn.GenIndyn.test_CallSiteNullErrorRethrown();
 			Assert.fail("BootstrapMethodError or NullPointerException should be thrown.");
-		} catch(BootstrapMethodError e) {
-			Assert.assertTrue(VersionCheck.major() >= 11);
+		} catch (BootstrapMethodError e) {
+			// Java 8 (with OJDK MHs): BoostrapMethodError is expected on the first run
+			// Java 11: BootstrapMethodError is expected on the first run
+			Assert.assertTrue(
+				(VersionCheck.major() >= 11)
+				|| "true".equals(System.getProperty("openjdk.methodhandles", "false")));
 		} catch (NullPointerException e) {
+			// Java 8 (with OJ9 MHs): NullPointerException is expected on the first run
 			Assert.assertTrue(VersionCheck.major() == 8);
 		}
 
 		// Java 8: CallSite resolution is expected to succeed
-		// Java 11 :The same BSME is expected on the second run
+		// Java 11: The same BSME is expected on the second run
 		try {
 			com.ibm.j9.jsr292.indyn.GenIndyn.test_CallSiteNullErrorRethrown();
 			Assert.assertTrue(VersionCheck.major() == 8);
-		} catch ( java.lang.BootstrapMethodError e ) {
+		} catch (java.lang.BootstrapMethodError e) {
 			Assert.assertTrue(VersionCheck.major() >= 11);			
 		}
 	}