8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info by calvinccheung · Pull Request #6070 · openjdk/jdk

calvinccheung · 2021-10-21T21:42:45Z

During dumping of CDS archive, the ArchiveBuilder::gather_klass_and_symbol set the address of an object to null if the
object cannot be archived. Since the lambda proxy class in this scenario contains an old version (major version < 50) of
interface which has been linked, the class won't be archived and its address will be set to null. The SystemDictionaryShared::adjust_lambda_proxy_class_dictionary is called after
ArchiveBuilder::gather_klass_and_symbol and thus encountered the null pointer.

A fix is to add a function SystemDictionaryShared::cleanup_lambda_proxy_class_dictionary to cleanup the
_dumptime_lambda_proxy_class_dictionary at the beginning of the CDS dumping operation.

The proposed fix passed tiers 1 - 4 testing (including the new tests).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info

Reviewers

Yumin Qi (@yminqi - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6070/head:pull/6070
$ git checkout pull/6070

Update a local copy of the PR:
$ git checkout pull/6070
$ git pull https://git.openjdk.java.net/jdk pull/6070/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6070

View PR using the GUI difftool:
$ git pr show -t 6070

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6070.diff

…proxy class info

calvinccheung · 2021-10-21T21:43:06Z

/label add hotspot-runtime

bridgekeeper · 2021-10-21T21:43:46Z

👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-10-21T21:45:50Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2021-10-21T21:58:33Z

Webrevs

yminqi

LGTM.

yminqi · 2021-10-22T16:57:19Z

src/hotspot/share/classfile/systemDictionaryShared.cpp


+class CleanupDumpTimeLambdaProxyClassTable: StackObj {
+ public:
+  CleanupDumpTimeLambdaProxyClassTable() {}


Why need a explicit public constructor for nothing?

Thanks for your review.
In a previous version of the fix, the _dumptime_lambda_proxy_class_dictionary was passed to the constructor but it is no longer needed. I've removed the constructor.

openjdk · 2021-10-22T17:04:26Z

@calvinccheung 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:

8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info

Reviewed-by: minqi, dholmes

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 161 new commits pushed to the master branch:

82f4aac: 8259609: C2: optimize long range checks in long counted loops
574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
71d593e: 8275162: Use varargs in 'def' macros in mutexLocker.cpp
7ca053d: 8251904: vmTestbase/nsk/sysdict/vm/stress/btree/btree010/btree010.java fails with ClassNotFoundException: nsk.sysdict.share.BTree0LLRLRLRRLR
4be88d5: 8275047: Optimize existing fill stubs for AVX-512 target
63e0f34: 8275767: JDK source code contains redundant boolean operations in jdk.charsets
4961373: 8275137: jdk.unsupported/sun.reflect.ReflectionFactory.readObjectNoDataForSerialization uses wrong signature
... and 151 more: https://git.openjdk.java.net/jdk/compare/d9e03e42afbb2e5115b67accfffad4938b8314b1...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dholmes-ora · 2021-10-25T00:32:23Z

src/hotspot/share/cds/metaspaceShared.cpp

 void VM_PopulateDumpSharedSpace::doit() {
  HeapShared::run_full_gc_in_vm_thread();

+  SystemDictionaryShared::start_dumping();


What is the significance of this? It seems unrelated to the bug being fixed.

It is for setting the _dump_in_progress flag to true so that no new entries will be added to the _dumptime_table.
The SystemDictionaryShared::start_dumping() is being called for the dynamic dumping case, it would be consistent
if it is also called for the static dumping case.

Perhaps but again that seems a different issue unrelated to this bug. Nor is it immediately obvious that the dynamic dumping and static dumping are equivalent in this regard. If the flag was only added for the dynamic case then apparently the static case does not need it - no?

I've reverted the change of calling SystemDictionaryShared::start_dumping() for the static dump case.
If the call is needed, it will be addressed via a separate bug.

…in metaspaceShared.cpp

dholmes-ora

Seems fine.

Thanks,
David

yminqi

Still LGTM.

calvinccheung · 2021-10-26T16:25:30Z

Thanks David and Yumin for the review.
/integrate

openjdk · 2021-10-26T16:27:11Z

Going to push as commit e5cd269.
Since your change was applied there have been 161 commits pushed to the master branch:

82f4aac: 8259609: C2: optimize long range checks in long counted loops
574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
71d593e: 8275162: Use varargs in 'def' macros in mutexLocker.cpp
7ca053d: 8251904: vmTestbase/nsk/sysdict/vm/stress/btree/btree010/btree010.java fails with ClassNotFoundException: nsk.sysdict.share.BTree0LLRLRLRRLR
4be88d5: 8275047: Optimize existing fill stubs for AVX-512 target
63e0f34: 8275767: JDK source code contains redundant boolean operations in jdk.charsets
4961373: 8275137: jdk.unsupported/sun.reflect.ReflectionFactory.readObjectNoDataForSerialization uses wrong signature
... and 151 more: https://git.openjdk.java.net/jdk/compare/d9e03e42afbb2e5115b67accfffad4938b8314b1...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-26T16:27:34Z

@calvinccheung Pushed as commit e5cd269.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda …

91f5b48

…proxy class info

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 21, 2021

fix trailing whitespace

81d53fe

calvinccheung marked this pull request as ready for review October 21, 2021 21:51

openjdk bot added the rfr Pull request is ready for review label Oct 21, 2021

yminqi approved these changes Oct 22, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2021

remove empty CleanupDumpTimeLambdaProxyClassTable ctor

13d5a86

dholmes-ora reviewed Oct 25, 2021

View reviewed changes

revert the change of calling SystemDictionaryShared::start_dumping() …

8432f32

…in metaspaceShared.cpp

dholmes-ora approved these changes Oct 25, 2021

View reviewed changes

yminqi approved these changes Oct 26, 2021

View reviewed changes

openjdk bot closed this Oct 26, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 26, 2021

calvinccheung deleted the 8274944-adjusting-lambda-proxy-classes branch October 26, 2021 16:29

Conversation

calvinccheung commented Oct 21, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

calvinccheung commented Oct 21, 2021

Uh oh!

bridgekeeper bot commented Oct 21, 2021

Uh oh!

openjdk bot commented Oct 21, 2021

Uh oh!

mlbridge bot commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

yminqi left a comment

Choose a reason for hiding this comment

Uh oh!

yminqi Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

calvinccheung Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

calvinccheung Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

calvinccheung Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

yminqi left a comment

Choose a reason for hiding this comment

Uh oh!

calvinccheung commented Oct 26, 2021

Uh oh!

openjdk bot commented Oct 26, 2021

Uh oh!

openjdk bot commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

calvinccheung commented Oct 21, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 21, 2021 •

edited

Loading

openjdk bot commented Oct 22, 2021 •

edited

Loading