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

Conform to spec for interface resolution for OJDK MHs #18476

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Conform to spec for interface resolution for OJDK MHs #18476

merged 1 commit into from
Nov 22, 2023

Conversation

ThanHenderson
Copy link
Contributor

This patch fixes #14984.
For Java 10-, private interface lookups should fail with a IncompatibleClassChangeError that is caught and wrapped in a IllegalAccessException. For OJ9 MHs, the IllegalAccessException is thrown in findInterface based on Java-code guards on method modifiers. For OJDK MHs, the IncompatibleClassChangeError is expected from a call to MethodHandleNatives.resolve and wrapped.

This patch ensures that the correct error is thrown for private interfaces during method lookup for Java 11- with OJDK MHs.

Issues: #14984
Signed-off-by: Nathan Henderson nathan.henderson@ibm.com

@ThanHenderson
Copy link
Contributor Author

ping: @babsingh

babsingh
babsingh reviewed Nov 20, 2023
View reviewed changes
runtime/vm/lookupmethod.c Outdated Show resolved Hide resolved
runtime/vm/lookupmethod.c Outdated Show resolved Hide resolved
runtime/vm/lookupmethod.c Outdated Show resolved Hide resolved
runtime/vm/lookupmethod.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

For OJ9 MHs, the IllegalAccessException is thrown in findInterface based on Java-code guards on method modifiers.

Modifying lookupmethod.c::processMethod might impact other areas, which are unrelated to MHs. Can you verify if this statement is true? See if any non-MH tests fail in a personal build.

If the above statement is true, is MemberName.resolve -> Java_java_lang_invoke_MethodHandleNatives_resolve a better place for error handling?

@ThanHenderson
Copy link
Contributor Author

Modifying lookupmethod.c::processMethod might impact other areas, which are unrelated to MHs. Can you verify if this statement is true? See if any non-MH tests fail in a personal build.

If the above statement is true, is MemberName.resolve -> Java_java_lang_invoke_MethodHandleNatives_resolve a better place for error handling?

I believe my changes are spec compliant regardless of MHs. I chose that location due to the existing check whose comment suggested that it was the appropriate location.

That being said, I'll verify with the personal builds and explore the efficacy of placing it directly in resolve.

@ThanHenderson
Conform to spec for interface resolution for OJDK MHs
adfdbd4 
This patch fixes eclipse-openj9#14984.
For Java 10-, private interface lookups should fail with a
IncompatibleClassChangeError that is caught and wrapped in an
IllegalAccessException. For OJ9 MHs, the IllegalAccessException
is thrown in findInterface based on Java-code guards on method
modifiers. For OJDK MHs, the IncompatibleClassChangeError is
expected from a call to MethodHandleNatives.resolve and wrapped.

This patch ensures that the correct error is thrown for private
interfaces during method lookup for Java 11- with OJDK MHs.

Issues: eclipse-openj9#14984
Signed-off-by: Nathan Henderson <nathan.henderson@ibm.com>
@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk zlinuxojdk292 jdk8,jdk11

@ThanHenderson
Copy link
Contributor Author

ThanHenderson commented Nov 22, 2023 •
edited
Loading

Failures for JDK11 are unrelated since this patch doesn't affect JDK11.

Failures in JDK8:

babsingh
babsingh approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

Only known failures are seen in the PR builds.

@babsingh babsingh merged commit a099118 into eclipse-openj9:master Nov 22, 2023
4 of 9 checks passed
@ThanHenderson ThanHenderson added comp:vm jdk8 jdk11 project:MH Used to track Method Handles related work and removed jdk11 labels Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@babsingh babsingh babsingh approved these changes

Assignees
No one assigned
Labels
comp:vm jdk8 project:MH Used to track Method Handles related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[JDK8 OJDK-MH] IllegalAccessException not thrown
2 participants
@ThanHenderson @babsingh