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

Update resolveConstantDynamic Error wrapping #2984

Merged
merged 1 commit into from
Sep 25, 2018
Merged

Update resolveConstantDynamic Error wrapping #2984

merged 1 commit into from
Sep 25, 2018

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Sep 21, 2018 •
edited
Loading

  • only wrap bootstrapMethod invoke & validation with try/catch
  • wrap non-Error exception with BootstrapMethodError

This is required by JVMS JDK11 Section 5.4.3.6 'Dynamically-Computed Constant and Call Site Resolution'

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@fengxue-IS
Copy link
Contributor Author

@JasonFengJ9 can you please take a look?

@JasonFengJ9
Copy link
Member

This change affects all Java versions including Java 8, please confirm that there is no JCK8 test regression introduced by this PR.

@fengxue-IS
Copy link
Contributor Author

This only affects Java 11 as cp entries that call resolveConstantDynamic is only valid starting Java 11

@JasonFengJ9
Copy link
Member

JasonFengJ9 commented Sep 22, 2018 •
edited
Loading

Added an new Throwable().printStackTrace(); at beginning of method java.lang.invoke.MethodHandle.resolveInvokeDynamic(), with following test code

    Consumer<String> c = System.out::println;
    c.accept("Hello!");

Run against latest Java 8 and got following stacktrace:

java.lang.Throwable
	at java.lang.invoke.MethodHandle.resolveInvokeDynamic(MethodHandle.java:899)
	at HelloWorld.main(HelloWorld.java:5)

This shows that modification to java.lang.invoke.MethodHandle.resolveInvokeDynamic might affect Java 8 as well.

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 22, 2018 •
edited
Loading

@JasonFengJ9 this PR is modifying ConstantDynamic, not invokeDynamic, please see line 852 for method name just above the first modification

JasonFengJ9
JasonFengJ9 approved these changes Sep 22, 2018
View reviewed changes
Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

Sorry, I mis-read a bit.
But, since j.l.i.MethodHandle.resolveConstantDynamic() is for Java 11 only, it should not present in Java 8, created #2986 .
This PR looks good.

@fengxue-IS
Copy link
Contributor Author

@DanHeidinga please take a look, thanks

DanHeidinga
DanHeidinga reviewed Sep 24, 2018
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

Invokedynamic resolution needs the same change in JDK11.

This is a change in behaviour for indy pre-jdk11.

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandle.java Outdated
staticArgs[staticArgIndex] = cpEntry;
}

try {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to this try block that references the appropriate part of the JVMS so that we can double check this behaviour in the future? It makes it much easier to figure out than trying to discern the subtle difference from the code

@fengxue-IS fengxue-IS changed the title Update resolveConstantDynamic BootstrapMethodError wrapping Update resolveConstantDynamic Error wrapping Sep 24, 2018
@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 24, 2018 •
edited
Loading

updated with reference to JVMS, also added a check so exception instance of Error class is not wrapped by extra layer of BootstrapMethodError.

JVMS 5.4.3.6: If the invocation fails by throwing an instance of Error or a subclass of Error, resolution fails with that exception.

InvokeDynamic will be addressed by #3001
@DanHeidinga can I get a re-review please

@fengxue-IS
Update resolveConstantDynamic Error wrapping
7cac939 
- only wrap bootstrapMethod invoke & validation with try/catch
- wrap non-Error exception with BootstrapMethodError

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
DanHeidinga
DanHeidinga approved these changes Sep 25, 2018
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm as the indy update will be covered under a separate issue

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk11

@DanHeidinga DanHeidinga self-assigned this Sep 25, 2018
@fengxue-IS fengxue-IS mentioned this pull request Sep 25, 2018
Merged
@DanHeidinga DanHeidinga merged commit b72aff7 into eclipse-openj9:master Sep 25, 2018
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

@JasonFengJ9 JasonFengJ9 JasonFengJ9 approved these changes

Assignees

@DanHeidinga DanHeidinga

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

Successfully merging this pull request may close these issues.
4 participants
@fengxue-IS @JasonFengJ9 @DanHeidinga @pshipton