From d60b28449bae291db558bac8f90637e00b1ce5e1 Mon Sep 17 00:00:00 2001 From: Nathan Henderson Date: Thu, 16 Nov 2023 14:36:53 -0800 Subject: [PATCH] Properly handle resolve-time invokeDynamic errors for OJDK MHs This patch fixes eclipse-openj9/openj9#14990. For OJDK MHs invokeDynamic resolve-time errors need to be wrapped in BootstrapMethodErrors and thrown during invoke-time. However, prior to Java 11, if a CallSite initially binds to null the error needs to be thrown during resolution to ensure that resolution is reattempted by the interpreter. This patch also updates the IndyTest.java test case to conform to the spec for OJDK MHs. Issues: eclipse-openj9/openj9#14990 Signed-off-by: Nathan Henderson --- .../lang/invoke/MethodHandleResolver.java | 50 ++++++++++++------- runtime/vm/BytecodeInterpreter.hpp | 38 ++++++-------- runtime/vm/resolvesupport.cpp | 29 ++++++----- .../src/com/ibm/j9/jsr292/indyn/IndyTest.java | 19 ++++--- 4 files changed, 72 insertions(+), 64 deletions(-) diff --git a/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java b/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java index 930837cd9af..45de513a65e 100644 --- a/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java +++ b/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$ } diff --git a/runtime/vm/BytecodeInterpreter.hpp b/runtime/vm/BytecodeInterpreter.hpp index a2eb85406ef..372927745cd 100644 --- a/runtime/vm/BytecodeInterpreter.hpp +++ b/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); diff --git a/runtime/vm/resolvesupport.cpp b/runtime/vm/resolvesupport.cpp index 9e7bc02c13b..666cffb5167 100644 --- a/runtime/vm/resolvesupport.cpp +++ b/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 */ diff --git a/test/functional/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java b/test/functional/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java index e2350f90eb1..79b4c224d68 100644 --- a/test/functional/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java +++ b/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); } }