8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes #118

tstuefe · 2022-01-25T09:20:30Z

Hi all,

not a clean backport. I had to fix up classLoaderData.cpp a bit because SystemDictionaryShared::handle_class_unloading(ik); does not yet exist in jdk17 and I did not want to bring the dependent cleanup change into jdk17 too (among other things because Oracle seems not to have done it either in their closed backport as far as I can glean from the JBS tags).

Tested in SAP nightlies for about a week now.

Thanks!

Progress

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

Issues

JDK-8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes
JDK-8274935: dumptime_table has stale entry

Reviewers

Goetz Lindenmaier (@GoeLin - Reviewer)
Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17u-dev pull/118/head:pull/118
$ git checkout pull/118

Update a local copy of the PR:
$ git checkout pull/118
$ git pull https://git.openjdk.java.net/jdk17u-dev pull/118/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 118

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17u-dev/pull/118.diff

bridgekeeper · 2022-01-25T09:21:02Z

👋 Welcome back stuefe! 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 · 2022-01-25T09:22:02Z

This backport pull request has now been updated with issues from the original commit.

mlbridge · 2022-01-25T09:27:05Z

Webrevs

00: Full (ab921275)

GoeLin · 2022-01-26T07:40:03Z

src/hotspot/share/classfile/classLoaderData.cpp

+      // But still have to remove it from the dumptime_table.
+      if (Arguments::is_dumping_archive()) {
+        SystemDictionaryShared::remove_dumptime_info(ik);
+      }


This is a part of handle_class_unloading(). I.e., you basically "undo" '8267189: Remove duplicated unregistered classes from dynamic archive'. That change replaced remove_dumptime_info() by handle_class_unloading() which calls remove_dumptime_info().
And you add the check for is_dumping_archive() that was
added to handle_class_unloading() in
'8265602: -XX:DumpLoadedClassList should support custom loaders'.

LGTM

@GoeLin I'm backporting https://bugs.openjdk.org/browse/JDK-8267189 to 17 and found this comment. Having trouble understanding it. Is it still safe / recommended to backport JDK-8267189 with this unclean 8274753 backport? Thanks.

Attempted to backport https://bugs.openjdk.org/browse/JDK-8267189, but the build failed due to these methods. I guess it won't be a clean backport.

Not very familiar with the code, would you be able to give suggestion on where I should update? With JDK-8267189, can we remove SystemDictionaryShared::remove_dumptime_info(ik);?

=== Output from failing command(s) repeated here === * For target hotspot_variant-server_libjvm_objs_classLoaderData.o: src/hotspot/share/classfile/classLoaderData.cpp: In member function 'void ClassLoaderData::free_deallocate_list_C_heap_structures()': src/hotspot/share/classfile/classLoaderData.cpp:892:56: error: 'static void SystemDictionaryShared::remove_dumptime_info(InstanceKlass*)' is private within this context SystemDictionaryShared::remove_dumptime_info(ik); ^ In file included from src/hotspot/share/classfile/classLoaderData.cpp:58:0: src/hotspot/share/classfile/systemDictionaryShared.hpp:234:15: note: declared private here static void remove_dumptime_info(InstanceKlass* k) NOT_CDS_RETURN; ^~~~~~~~~~~~~~~~~~~~ At global scope: cc1plus: error: unrecognized command line option '-Wno-cast-function-type' [-Werror] cc1plus: all warnings being treated as errors * All command lines available in /workplace/ruiamzn/github/jdk17u-dev/build/linux-x86_64-server-release/make-support/failure-logs. === End of repeated output ===

Looked through the code, JDK-8267189 introduces handle_class_unloading.

It seems undoing this change necessary changes for jdk17 as part of backporting JDK-8267189 makes sense (?). Tested locally with simply make images, can build.

Tested jtreg_tier1 with current draft, failed at TestUnloadClassError.java

Please don't comment in this old PR. Use the new one you opened.

openjdk · 2022-01-26T07:42:44Z

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

8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes
8274935: dumptime_table has stale entry

Reviewed-by: goetz, phh

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

49cf332: 8251216: Implement MD5 intrinsics on AArch64
9378f26: 8278241: Implement JVM SpinPause on linux-aarch64
a93e5ef: 8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow
096c31c: 8276057: Update JMH devkit to 1.33
0cc9d30: 8275082: Update XML Security for Java to 2.3.0
3d6f331: 8177814: jdk/editpad is not in jdk TEST.groups
0a1419c: 8253197: vmTestbase/nsk/jvmti/StopThread/stopthrd007/TestDescription.java fails with "ERROR: DebuggeeSleepingThread: ThreadDeath lost"
59d39ef: 8236505: Mark jdk/editpad/EditPadTest.java as @headful
28dcaf3: 8272553: several hotspot runtime/CommandLine tests don't check exit code
6315ce3: 8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException
... and 1 more: https://git.openjdk.java.net/jdk17u-dev/compare/55fb592e455ab87c38f17c0e746641a978b88cf3...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.

phohensee

Lgtm.

tstuefe · 2022-01-27T08:52:18Z

@GoeLin, @phohensee thanks!
@GoeLin could you approve the fix request then?

tstuefe · 2022-01-27T13:32:00Z

/integrate

openjdk · 2022-01-27T13:34:07Z

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

49cf332: 8251216: Implement MD5 intrinsics on AArch64
9378f26: 8278241: Implement JVM SpinPause on linux-aarch64
a93e5ef: 8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow
096c31c: 8276057: Update JMH devkit to 1.33
0cc9d30: 8275082: Update XML Security for Java to 2.3.0
3d6f331: 8177814: jdk/editpad is not in jdk TEST.groups
0a1419c: 8253197: vmTestbase/nsk/jvmti/StopThread/stopthrd007/TestDescription.java fails with "ERROR: DebuggeeSleepingThread: ThreadDeath lost"
59d39ef: 8236505: Mark jdk/editpad/EditPadTest.java as @headful
28dcaf3: 8272553: several hotspot runtime/CommandLine tests don't check exit code
6315ce3: 8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException
... and 1 more: https://git.openjdk.java.net/jdk17u-dev/compare/55fb592e455ab87c38f17c0e746641a978b88cf3...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-27T13:34:34Z

@tstuefe Pushed as commit b23271b.

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

duke and others added 2 commits January 24, 2022 05:23

Backport 110e38ded8e09361f24c582c770d35f5cfdabf82

4b1ea51

necessary changes for jdk17

ab92127

tstuefe changed the title ~~tstuefe backport 110e38de~~ Backport 110e38ded8e09361f24c582c770d35f5cfdabf82 Jan 25, 2022

openjdk bot changed the title ~~Backport 110e38ded8e09361f24c582c770d35f5cfdabf82~~ 8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes Jan 25, 2022

openjdk bot added backport Port of a pull request already in a different code base rfr Pull request is ready for review labels Jan 25, 2022

GoeLin approved these changes Jan 26, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 26, 2022

phohensee approved these changes Jan 26, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 27, 2022

openjdk bot closed this Jan 27, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 27, 2022

rgithubli mentioned this pull request Oct 24, 2022

8267189: Remove duplicated unregistered classes from dynamic archive #819

Closed

3 tasks

8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes #118

8274753: ZGC: SEGV in MetaspaceShared::link_shared_classes #118

Uh oh!

Conversation

tstuefe commented Jan 25, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 25, 2022

Uh oh!

openjdk bot commented Jan 25, 2022

Uh oh!

mlbridge bot commented Jan 25, 2022

Webrevs

Uh oh!

GoeLin Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgithubli Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

rgithubli Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgithubli Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgithubli Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

GoeLin Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phohensee left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Jan 27, 2022

Uh oh!

tstuefe commented Jan 27, 2022

Uh oh!

openjdk bot commented Jan 27, 2022

Uh oh!

openjdk bot commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

tstuefe commented Jan 25, 2022 •

edited by openjdk bot

Loading

GoeLin Jan 26, 2022 •

edited

Loading

rgithubli Oct 21, 2022 •

edited

Loading

rgithubli Oct 21, 2022 •

edited

Loading

openjdk bot commented Jan 26, 2022 •

edited

Loading