RFR: 8304147: JVM crash during shutdown when dumping dynamic archive [v2]

[Coleen Phillimore]

On Wed, 22 Mar 2023 22:17:27 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> `DynamicArchive::prepare_for_dump_at_exit()` could be called concurrently on the two VM exit paths, leading either to assertion failures in debug builds, or a crash in product build when the actual dump occurred. The basic fix was to move the `DynamicArchive::prepare_for_dump_at_exit()` functionality into `before_exit` (java.cpp) where it can only be executed by one thread - thus avoiding the race (Phase 1).
>> 
>> Once that is done we can move the actual dump operation to be co-located with the `prepare_for_dump_at_exit()` (Phase 2) and then consolidate that code by refactoring it into a new `DynamicArchive::dump_at_exit()` method (Phase 3), and then removing the leftover methods that are no longer needed (Phase 4).
>> 
>> Each phase can be seen in distinct commits if you prefer to see the steps involved.
>> 
>> Testing:
>>  - new ExitRace test
>>  - all dynamicArchive tests
>>  - tiers 1-4
>> 
>> Thanks.
>
> David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
> 
>  - Restore comment to as it was pre JDK-8266770
>  - Update test to removing debugging code
>  - Merge branch 'master' into 8304147-dynamic-dump
>  - Update test comments
>  - Phase 4: remove unused methods and adjust dump_for_jcmd
>  - Missed removing the check at the dump_at_exit callsite.
>  - Phase 3: Consolidate code into a new dump_at_exit that combines check, prepare and dump in one.
>    Minor tweak to test.
>  - Merge branch '8304147-test-only' into 8304147-dynamic-dump
>  - Initial test
>  - Phase 2: Move the dump to immediately after the prepare_for_dump
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/a3c7e999...518f5409

This is a very nice simplification of the JVM exit paths. I had a few minor comments.

src/hotspot/share/cds/dynamicArchive.cpp line 383:

> 381:   ExceptionMark em(current);
> 382:   ResourceMark rm(current);
> 383:   HandleMark hm(current);

Why is there a HandleMark here?  Should it be lower in the call stack?

src/hotspot/share/cds/dynamicArchive.cpp line 385:

> 383:   HandleMark hm(current);
> 384: 
> 385:   if (!(DynamicDumpSharedSpaces && archive_name != nullptr)) {

Can you make this !DynamicDumpSharedSpaces || archive_name == nullptr so it's easier to read.  I'm trying to figure out how archive_name relates to ArchiveClassesAtExit and why you made the switch.

src/hotspot/share/cds/dynamicArchive.cpp line 389:

> 387:   }
> 388: 
> 389:   log_info(cds,dynamic)("Preparing for dynamic dump at exit in thread %s", current->name());

nit, needs a ' ' between cds, dynamic.

src/hotspot/share/cds/dynamicArchive.cpp line 391:

> 389:   log_info(cds,dynamic)("Preparing for dynamic dump at exit in thread %s", current->name());
> 390: 
> 391:   MetaspaceShared::link_shared_classes(false/*not from jcmd*/, current);

link_shared_classes is declared with TRAPS, so this should have a
JavaThread* THREAD = current;
and use HAS_PENDING_EXCEPTION, CLEAR_PENDING_EXCEPTION macros.

-------------

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13134#pullrequestreview-1357247610
PR Review Comment: https://git.openjdk.org/jdk/pull/13134#discussion_r1147940650
PR Review Comment: https://git.openjdk.org/jdk/pull/13134#discussion_r1147939836
PR Review Comment: https://git.openjdk.org/jdk/pull/13134#discussion_r1147940076
PR Review Comment: https://git.openjdk.org/jdk/pull/13134#discussion_r1147937042