Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly handle resolve-time invokeDynamic errors for OJDK MHs #18474

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Properly handle resolve-time invokeDynamic errors for OJDK MHs #18474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 31 additions & 19 deletions jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)]*/
Expand All @@ -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;
Comment on lines +308 to 315
Copy link
Contributor

@babsingh babsingh Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a potential improvement to remove local variables (resultHandle and memberName) and reduce code size for the future:

	result[1] = MethodHandles.foldArguments(thrower, combiner);
/*[IF JAVA_SPEC_VERSION >= 11]*/
	result[0] = result[1].internalForm().vmentry;
/*[ELSE] JAVA_SPEC_VERSION >= 11*/
	result[0] = result[1].internalForm().compileToBytecode();
/*[ENDIF] JAVA_SPEC_VERSION >= 11*/

} catch (IllegalAccessException iae) {
Expand All @@ -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;
Expand All @@ -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$
}
Expand Down
38 changes: 16 additions & 22 deletions runtime/vm/BytecodeInterpreter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
29 changes: 14 additions & 15 deletions runtime/vm/resolvesupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
19 changes: 11 additions & 8 deletions test/functional/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
ThanHenderson marked this conversation as resolved.
Show resolved Hide resolved
} 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);
}
}
Expand Down