Update linkToInterface for OpenJDK MethodHandles

[babsingh]

Now, it properly handles:

• Private interface methods
• j.l.Object methods
• vTable offset for the method
• iTable index for the method

Also, the latest version of the class is retrieved for the iTable search.

Co-authored-by: Jack Lu Jack.S.Lu@ibm.com
Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

[babsingh]

fyi @DanHeidinga

[DanHeidinga]

This fetches J9JNIMethodID* from vmindexOffset and then tries to decode it for an interface. Looking at the code that initializes the J9JNIMethodID:
https://github.com/eclipse/openj9/blob/a7b7dbe51e2c8801b4f0a22bb98322d7dda76692/runtime/vm/jnicsup.cpp#L2097-L2122

it explicitly states that it doesn't tag the vTableIndex for static interface methods or methods from Object as J9_JNI_MID_INTERFACE.

If it's one of those cases, the code below that looks up the method will be wrong:

_sendMethod = *(J9Method **)(((UDATA)receiverClass) + vTableOffset);

Can you point me at the code that ensures we won't try to run linkToInterface for private interface methods or for Object methods looked up on the interface?

With the OpenJ9 implementation, we handled these cases in the Java code that created the MH by returning DirectHandles or VirtualHandles wrapped with appropriate asTypes to ensure we couldn't hit them in the native code.

[babsingh]

[1] DirectMethodHandle that handles invokeinterface: has viewAsType; has checkReceiver, which gets called before linkToInterface.
[2] DirectMethodHandle check for invokeinterface of Object methods.
[3] In [2], MethodHandleNatives.resolve (native code) is used to generate MemberName.

Unsure if I have identified the correct code. +@fengxue-IS for assist.

@DanHeidinga can you please provide a link to the corresponding J9 code?

[DanHeidinga]

Look at
https://github.com/eclipse/openj9/blob/c757b51154427a436921984a8a4cb316bb5c6cf6/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java#L839-L855

And at
https://github.com/eclipse/openj9/blob/c757b51154427a436921984a8a4cb316bb5c6cf6/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java#L1077

[DanHeidinga]

[2] looks like it covers the method from object case.

Writing a test - or finding an existing one - would be a good way to validate the various cases here

[babsingh]

Writing a test - or finding an existing one - would be a good way to validate the various cases here
We won't try to run linkToInterface for private interface methods or for Object methods looked up on the interface

I ran tests, which are attached here: Test.java. They ...

• verify if IllegalAccessError is thrown if a package private implementation of an interface method is invoked.
• verify if the correct Object method is invoked when the lookup is done using an interface class.

The above tests pass.

W.r.t. the OJDK code: The code mentioned in #11371 (comment) is also used in the above tests for generating exceptions.

1. MemberName resolution code-path: LINK to OJDK code.
2. Generation of exceptions: LINK to OJDK code.

[DanHeidinga]

Thanks for putting those tests together. The case I was thinking of was a private method on the interface being invoked directly:

jshell> import java.lang.invoke.*;

jshell> interface I { private void private_m() { System.out.println("private_m"); } static MethodHandles.Lookup l() { return MethodHandles.lookup(); } }

jshell> I.l().findVirtual(I.class, "private_m", MethodType.methodType(void.class));
$5 ==> MethodHandle(I)void

jshell> $5.invoke(new I() {})
private_m
$7 ==> null

This should map to the equivalent of an invokeinterface I.private_m() which would become the direct method be tagged as such

[babsingh]

private method on the interface being invoked directly

The above example fails. linkToInterface is invoked for private interface methods. _sendMethod needs to be derived properly for private interface methods as per LINK.

Object methods looked up on the interface

@DanHeidinga Can you confirm if the below example is correct for object methods looked up on the interface?

class Test {
	public static void main(String[] args) throws Throwable {
		TestClass_IH clazz = new TestClass_IH();
		mh = MethodHandles.lookup().findVirtual(TestInterface.class, "test", MethodType.methodType(void.class));
		System.out.println("interface class: " + mh.getClass().toString());
		mh.invokeExact((TestInterface)clazz);
	}

	interface TestInterface {
		default void test() {
			System.out.println("Fail: invoking the default interface method.");
		}
	}

	static class TestClass_IH implements TestInterface {
		public void test() {
			System.out.println("Pass: invoking the correct object method.");		
		}	
	}
}

[DanHeidinga]

@DanHeidinga Can you confirm if the below example is correct for object methods looked up on the interface?

No, it's not. The case is for java.lang.Object methods, not any object. Something like:

jshell> import java.lang.invoke.*;
jshell> interface I { private void private_m() { System.out.println("private_m"); } static MethodHandles.Lookup l() { return MethodHandles.lookup(); } }
jshell> I.l().findVirtual(I.class, "toString", MethodType.methodType(String.class))
$8 ==> MethodHandle(I)String
jshell> $8.invoke(new I() {})
$9 ==> "$1@76ccd017"

The interface extends from Object and so can call Object's methods even though the interface doesn't declare them and there's no ITable for them.

Another good example would be wait as that's final in Object and won't be in the vtable:

jshell> I.l().findVirtual(I.class, "wait", MethodType.methodType(void.class))
$11 ==> MethodHandle(I)void

[babsingh]

Confirming that linkToInterface will need to handle both private interface methods and j.l.Object methods looked up on the interface.

[babsingh]

@DanHeidinga The PR has been updated to correctly handle private interface methods, j.l.Object methods, vTable offset for the method and iTable index for the method.

I don't think jnicsup.cpp::initializeMethodID will return a vTable offset with J9_ITABLE_OFFSET_TAG_BITS set. So, we do not need to address #10733 (comment).

I reran the tests, Test.java. OpenJ9 with OpenJDK MHs enabled behaves identically to RI.

[DanHeidinga]

The MemberName->vmtargetOffset is directly holding onto a J9Method*. How is that pointer updated during HCR? I'm concerned that the loop through the iTable may fail to match the interfaceClass if HCR has replaced the class with a newer version. (ie: The cached J9Method * may hold onto an obsolete class pointer)

[babsingh]

Should VM_VMHelpers::currentClass get the most current version of class after HCR?

Suggested change

	J9Class *interfaceClass = J9_CLASS_FROM_METHOD(method);
	J9Class *interfaceClass = VM_VMHelpers::currentClass(J9_CLASS_FROM_METHOD(method));

Do we also need to update the J9Method ref stored in MemberName for HCR? Related code:

• hshelp.c::fixDirectHandles: Here J9Methods stored in OpenJ9 MHs are updated. We probably need to do the same for MemberNames->vmtarget (J9Method).
• hshelp.c::fixJNIRefs will need to be invoked before fixing MemberNames->vmindex (J9JNIMethodID).
• jvmtiClass.c::redefineClassesCommon: invokes hshelp.c::fixJNIRefs and hshelp.c::fixDirectHandles.
• Related question: Implement MethodHandleNatives native code #10690 (comment)

[DanHeidinga]

Should VM_VMHelpers::currentClass get the most current version of class after HCR?

This solves half the problem in that it ensures the most up-to-date version of the class is used in the itable search. Unfortunately, the itableIndex may not point to the same method in the new version of the class if the methods have been reordered.

It's really two pieces of data - class version & itable index - and both need to kept consistent and up to date.

• hshelp.c::fixDirectHandles: Here J9Methods stored in OpenJ9 MHs are updated. We probably need to do the same for MemberNames->vmtarget (J9Method).

This is carefully set up thru cooperation between the MH creation java code & the redef code to avoid a full heap walk to find the DirectHandles. We may be able to make the same invariants hold for MemberName but I'm less clear on how to do that without patching the OJDK java code.

Couple of options here:

1. Do a full heap walk on redefinition to fix the vmtarget & vmindex fields of the MemberName. This will regress the use cases we fixed by adding the DirectHandle cache but it will be correct.

2. Change vmindex to be an index into class->jniIDs table. This should let the existing mechanisms fix the JNIID when the class is redefined but will require changes to the way invocation occurs. We'll also need to figure out how to update vmtarget as well.

3. Something else?

I'd suggest starting with option 1 as it's the easiest to get right and we can figure out how to optimize it later.

[babsingh]

I have updated this PR to handle the VM_VMHelpers::currentClass change. For the bigger part, I have opened the following issue: #11528. This PR is ready to be reviewed and merged. The bigger part will be handled in a separate PR.

[DanHeidinga]

I'm not going to run a build on this given the travis build has passed and the code is all under ifdef anyway

[DanHeidinga]

This is a step in the right direction and with the opening of #11528 to track the HCR-related issues, I'm OK with merging this.