-
Notifications
You must be signed in to change notification settings - Fork 58
8290417: CDS cannot archive lamda proxy with useImplMethodHandle #146
8290417: CDS cannot archive lamda proxy with useImplMethodHandle #146
Conversation
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Thanks.
test/hotspot/jtreg/runtime/cds/appcds/LambdaWithUseImplMethodHandle.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicLambdaWithUseImplMethodHandle.java
Outdated
Show resolved
Hide resolved
@iklam This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicLambdaWithUseImplMethodHandle.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/runtime/cds/appcds/test-classes/pkg2/Child.jcod
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just couple of nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. I agree that this fix is low risk to disable archiving the lambda proxy in the case when passing a live method handle for invocation.
test/hotspot/jtreg/runtime/cds/appcds/test-classes/pkg1/BaseWithProtectedMethod.java
Show resolved
Hide resolved
Thanks @dholmes-ora, @calvinccheung, @mlchung for the review. |
Going to push as commit 4dc421c.
Your commit was automatically rebased without conflicts. |
Since the impact of this bug can be big, and the fix is simple and low risk (just avoids doing optimizations), I'd like to put this into JDK 19 before RDP2 (Jul 21).
CDS cannot handle Lambda proxy classes that are generated in the useImplMethodHandle mode. This could happen with classfiles generated by JDK 8 or JDK 11 that access protected methods in a base class from a different package. E.g.,
More details of useImplMethodHandle can be found here: https://github.com/openjdk/jdk/blob/522b65743ca10fcba0a27d25b8fa11319999e228/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L183-L191
More details about the condition that triggers the error can be found in the test file Child.jcod.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/146/head:pull/146
$ git checkout pull/146
Update a local copy of the PR:
$ git checkout pull/146
$ git pull https://git.openjdk.org/jdk19 pull/146/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 146
View PR using the GUI difftool:
$ git pr show -t 146
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/146.diff