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

Handle interface special cases for MethodHandle #2860

Merged
merged 1 commit into from
Sep 14, 2018
Merged

Handle interface special cases for MethodHandle #2860

merged 1 commit into from
Sep 14, 2018

Conversation

gacholio
Copy link
Contributor

@gacholio gacholio commented Sep 13, 2018 •
edited
Loading

  • the final (non-virtual Object method) case was not being handled
    correctly in adaptInterfaceLookupsOfObjectMethodsIfRequired.

  • findVirtual of private interface methods allowed in JDK11 and beyond

  • unreflect of private interface methods allowed in JDK9 and beyond

  • invoking a private implementation of an interface method results in
    AbstractMethodError in JDK11 and beyond, IllegalAccessError before

  • fix interpreter to only throw AbstractMethodError for private implementations of interface methods in JDK11

  • update tests according to behaviour changes after JDK8

  • document that
    Java_java_lang_invoke_InterfaceHandle_convertITableIndexToVTableIndex
    does not need to deal with any special case interface invocations

Fixes: #2811

[ci skip]

Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com

@gacholio gacholio mentioned this pull request Sep 13, 2018
Closed
@gacholio
Copy link
Contributor Author

This is the reverted #2774 with fixes and matching test updates.

@gacholio
Copy link
Contributor Author

Using unmodified test code.

JDK11:

PASSED: test_bindTo_interfaceMethodImplementation_package
PASSED: test_bindTo_interfaceMethodImplementation_protected
PASSED: test_bindTo_interfaceMethodImplementation_public
FAILED: test_bindTo_interfaceMethodImplementation_private
java.lang.AbstractMethodError
at com.ibm.j9.jsr292.InterfaceHandleTest.test_bindTo_interfaceMethodImplementation_private(InterfaceHandleTest.java:115)

JDK10:

PASSED: test_bindTo_interfaceMethodImplementation_package
PASSED: test_bindTo_interfaceMethodImplementation_private
PASSED: test_bindTo_interfaceMethodImplementation_protected
PASSED: test_bindTo_interfaceMethodImplementation_public

So, invoking private implementation of an interface method results in AbstractMethodError in JDK11, but IllegalAccessError in previous JDKs.

default and protected result in IllegalAccessError everywhere.

@gacholio
Handle interface special cases for MethodHandle
e449801 
- the final (non-virtual Object method) case was not being handled
correctly in adaptInterfaceLookupsOfObjectMethodsIfRequired.

- findVirtual of private interface methods allowed in JDK11 and beyond

- unreflect of private interface methods allowed in JDK9 and beyond

- invoking a private implementation of an interface method results in
AbstractMethodError in JDK11 and beyond, IllegalAccessError before

- fix interpreter to only throw AbstractMethodError for private
implementations of interface methods in JDK11

- update tests according to behaviour changes after JDK8

- document that
Java_java_lang_invoke_InterfaceHandle_convertITableIndexToVTableIndex
does not need to deal with any special case interface invocations
 
[ci skip]

Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
@gacholio
Copy link
Contributor Author

jenkins test all xlinux jdk8,jdk10

DanHeidinga
DanHeidinga approved these changes Sep 14, 2018
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm.

I think we'll need to look at the MethodHandles.revealDirect() to make sure the MHInfo returned is correct for all 3 cases of interfacehandles. That can be done as a separate task

https://github.com/eclipse/openj9/blob/099779d6d40d424e1637390a4584757502cb6ca5/jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java#L1502

@DanHeidinga DanHeidinga self-assigned this Sep 14, 2018
@gacholio gacholio requested a review from llxia September 14, 2018 03:15
@gacholio gacholio changed the title WIP: Handle interface special cases for MethodHandle Handle interface special cases for MethodHandle Sep 14, 2018
@DanHeidinga
Copy link
Member

jenkins test sanity xlinux jdk11

@DanHeidinga
Copy link
Member

Jenkins test extended plinux jdk11

@gacholio
Copy link
Contributor Author

JDK8 extended failed "TestAttachErrorHandling_SE80_0", which is obviously an intermittent failure unrelated to this PR.

@gacholio
Copy link
Contributor Author

10 failing "TestAttachErrorHandling_0" which is clearly the same thing.

@gacholio
Copy link
Contributor Author

JDK11 sanity failed for what looks like flaky filesystem, again nothing to do with this PR.

@gacholio
Copy link
Contributor Author

The 292 tests have passed in 11 extended.

@gacholio
Copy link
Contributor Author

@llxia Please review the test changes (the build changes were provided by you!)

@gacholio
Copy link
Contributor Author

The JSR292 failure in JDK11 extended is also a flaky filesystem. I vaguely remember a time when tests actually passed on a regular basis.

@gacholio
Copy link
Contributor Author

Despite the fact that pretty much every test job failed, this PR is ready for merge once approved.

@pdbain-ibm
Copy link
Contributor

Investigating attach API failures under #2871.

@DanHeidinga
Copy link
Member

@llxia Can you review the tests? we want this merged for jdk11

@DanHeidinga DanHeidinga merged commit 9f53aac into eclipse-openj9:master Sep 14, 2018
@gacholio gacholio deleted the mh branch September 14, 2018 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga approved these changes

@llxia llxia llxia approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:test comp:vm jdk11
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_bindTo_interfaceMethodImplementation_* tests failed in JDK11
5 participants
@gacholio @DanHeidinga @pdbain-ibm @llxia @pshipton