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

merged 1 commit into from
Nov 22, 2023

Conversation

ThanHenderson
Copy link
Contributor

This patch fixes #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: #14990
Signed-off-by: Nathan Henderson [email protected]

@ThanHenderson
Copy link
Contributor Author

As a side effect, this patch also addresses the second point here: #10913 meaning the interpreter no longer has to check whether it has a Throwable or a [MemberName, Appendix] array.

@ThanHenderson
Copy link
Contributor Author

ping: @babsingh

@babsingh
Copy link
Contributor

  • What testing has been run on these changes? Can you provide me a link to the personal build?
  • There is unprocessed markdown in the commit message: 9a18b0a. The commit message will be easier to read without the markdown. This doesn't apply to the PR description.

@ThanHenderson
Copy link
Contributor Author

What testing has been run on these changes?

Last week I was having issues with jenkins aborting builds with and without my patch. I can launch them again today with JDK 8, 11, and 17 since these changes will affect all of them.

@babsingh
Copy link
Contributor

Last week I was having issues with jenkins aborting builds with and without my patch. I can launch them again today with JDK 8, 11, and 17 since these changes will affect all of them.

I can launch them in the PR. Was planning to minimize PR testing if prior testing was already done.

@ThanHenderson
Copy link
Contributor Author

ThanHenderson commented Nov 20, 2023 •
edited
Loading

Last week I was having issues with jenkins aborting builds with and without my patch. I can launch them again today with JDK 8, 11, and 17 since these changes will affect all of them.

I can launch them in the PR. Was planning to minimize PR testing if prior testing was already done.

JDK 17 completed fine in Jenkins (https://hyc-runtimes-jenkins.swg-devops.com/job/Pipeline-Build-Test-Personal/19247/). But the grinder aborted, probably best to re-run anyways.

@ThanHenderson ThanHenderson force-pushed the 14990 branch 2 times, most recently from 5fce658 to 8e440bf Compare November 21, 2023 18:38
@ThanHenderson
Properly handle resolve-time invokeDynamic errors for OJDK MHs
d60b284 
This patch fixes eclipse-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#14990
Signed-off-by: Nathan Henderson <[email protected]>
@babsingh
Copy link
Contributor

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

@babsingh
Copy link
Contributor

jenkins test sanity.functional,extended.functional,sanity.openjdk,extended.openjdk plinux jdk8,jdk17

@ThanHenderson
Copy link
Contributor Author

JDK11 failures:

JDK8 failures:

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, which are unrelated to these changes, are seen in the PR builds.

@babsingh babsingh merged commit 9563621 into eclipse-openj9:master Nov 22, 2023
babsingh
babsingh reviewed Nov 22, 2023
View reviewed changes
jcl/src/java.base/share/classes/java/lang/invoke/MethodHandleResolver.java
Comment on lines +308 to 315
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;
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*/

@ThanHenderson ThanHenderson added jdk8 comp:vm jdk11 project:MH Used to track Method Handles related work 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 jdk11 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] test_CallSiteNullErrorRethrown fails
2 participants
@ThanHenderson @babsingh