8320331: G1 Full GC Heap verification relies on metadata not reset before verification #16733

tschatzl · 2023-11-20T11:24:12Z

Hi all,

please review this fix to full gc to properly set parsable_bottom for the heap regions marked through so that "during" verification (-XX:+VerifyDuringGC) does not access metadata already unlinked but not yet completely purged.

The test case reproduces crashes fairly well without the patch; the new test case is run with debug VMs only purely to save testing time.

Testing: gha, test case

Thanks,
Thomas

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8320331: G1 Full GC Heap verification relies on metadata not reset before verification (Bug - P4)

Reviewers

Ivan Walulya (@walulyai - Reviewer) ⚠️ Review applies to 15812e5c
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16733/head:pull/16733
$ git checkout pull/16733

Update a local copy of the PR:
$ git checkout pull/16733
$ git pull https://git.openjdk.org/jdk.git pull/16733/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16733

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16733.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-20T11:25:26Z

👋 Welcome back tschatzl! 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.

tschatzl · 2023-11-20T11:26:29Z

/label add hotspot-gc

openjdk · 2023-11-20T11:26:46Z

@tschatzl
The hotspot-gc label was successfully added.

mlbridge · 2023-11-20T11:31:50Z

Webrevs

01: Full - Incremental (cc23c2d8)
00: Full (15812e5c)

walulyai

LGTM!

Minor:

update the comment associated with _parsable_bottom
Add a comment to prepare_for_full_gc to hint on why this is required.

openjdk · 2023-11-22T10:17:18Z

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

8320331: G1 Full GC Heap verification relies on metadata not reset before verification

Reviewed-by: iwalulya, ayang

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

5e81831: 8320399: RISC-V: Some format clean-up in opto assembly code
ceb0290: 8320447: Remove obsolete LintCategory.hidden
98edb03: 8320382: Remove CompressedKlassPointers::is_valid_base()
2c31ca5: 8320335: Deprecate RegisterFinalizersAtInit option and code
b3616c9: 8320131: Zero build fails on macOS after JDK-8254693
d1af748: 8309067: gtest/AsyncLogGtest.java fails again in stderrOutput_vm
e47cf61: 8074211: javax.sound.midi: Error with send System Exclusive messages of different length
6d82436: 8320278: ARM32 build is broken after JDK-8301997
f69e665: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout
1c0bd81: 8319124: Update XML Security for Java to 3.0.3
... and 34 more: https://git.openjdk.org/jdk/compare/6c5e15c1a291ca5ba1e4c3a90351bc71665ce988...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.

albertnetymk · 2023-11-22T11:10:54Z

src/hotspot/share/gc/g1/heapRegion.inline.hpp

@@ -167,6 +167,13 @@ inline size_t HeapRegion::block_size(const HeapWord* p, HeapWord* const pb) cons
  return cast_to_oop(p)->size();
 }

+inline void HeapRegion::prepare_for_full_gc() {


I don't believe this API should be part of this class. It appears entirely caller-specific, and it's unclear why the implementation is just a field-update without knowledge of the caller context. While the comment helps mitigate the situation, making it acceptable for a bugfix, I think it would be more appropriate to relocate methods related to parsable-bottom outside of this class.

I don't believe this API should be part of this class. It appears entirely caller-specific,

All of these _for_full_gc/_after_full_gc methods (and note_*_marking/scrubbing methods for concurrent mark) are caller specific, for exactly one caller, for a particular situation the caller encounters. Is there something particularly problematic with this exact method compared to others?

and it's unclear why the implementation is just a field-update without knowledge of the caller context. While the comment helps mitigate the situation, making it acceptable for a bugfix, I think it would be more appropriate to relocate methods related to parsable-bottom outside of this class.

So basically _parsable_bottom (and _top_at_mark_start since it is very similar to PB) fields should be somewhere else then? I think there is a CR for moving TAMS somewhere else, there is nothing for PB.

Is there something particularly problematic with this exact method compared to others?

No; all of them require caller-info to understand the impl.

fields should be somewhere else then?

Yes, because they are constructed/maintained by sth external to heap-region, IMO.

tschatzl · 2023-11-22T17:16:52Z

Thanks @albertnetymk @walulyai for your reviews
/integrate

openjdk · 2023-11-22T17:17:13Z

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

93bdc2a: 8306055: Add a built-in Catalog to JDK XML module
a4bd9e4: 8319440: RISC-V: jdk can't be built with clang due to register keyword
524da14: 8320418: PPC64: invokevfinal_helper duplicates code to handle ResolvedMethodEntry
5d4a54b: 8319449: compiler/print/CompileCommandPrintMemStat.java fails on Graal
35526d0: 8257076: os::scan_pages is empty on all platforms
25cebe8: 8317799: AIX PPC64: FFI symbol lookup doesn't find symbols
c39d001: 8319137: release _object in ObjectMonitor dtor to avoid races
8b47a14: 8320309: AIX: pthreads created by foreign test library don't work as expected
bf0a904: 8318175: AIX PPC64: Handle alignment of double in structs
6ce0ebb: 8295343: sun/security/pkcs11 tests fail on Linux RHEL 8.6 and newer
... and 44 more: https://git.openjdk.org/jdk/compare/6c5e15c1a291ca5ba1e4c3a90351bc71665ce988...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-22T17:17:19Z

@tschatzl Pushed as commit 1629a90.

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

Thomas Schatzl added 2 commits November 20, 2023 12:20

8320331 g1 full gc "during" verification accesses half-unloaded metadata

ce47d95

Only run test case on debug VMs, sufficient

15812e5

openjdk bot changed the title ~~8320331~~ 8320331: G1 Full GC Heap verification relies on metadata not reset before verification Nov 20, 2023

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Nov 20, 2023

tschatzl marked this pull request as ready for review November 20, 2023 11:26

openjdk bot added the rfr Pull request is ready for review label Nov 20, 2023

openjdk bot mentioned this pull request Nov 21, 2023

8317809: Insertion of free code blobs into code cache can be very slow during class unloading #16759

Closed

3 tasks

walulyai approved these changes Nov 22, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 22, 2023

iwaluya review, comments

cc23c2d

albertnetymk approved these changes Nov 22, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 22, 2023

openjdk bot closed this Nov 22, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 22, 2023

tschatzl deleted the submit/8320331-full-gc-heap-verification branch January 16, 2024 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320331: G1 Full GC Heap verification relies on metadata not reset before verification #16733

8320331: G1 Full GC Heap verification relies on metadata not reset before verification #16733

tschatzl commented Nov 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 20, 2023

tschatzl commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

mlbridge bot commented Nov 20, 2023 •

edited

Loading

walulyai left a comment

openjdk bot commented Nov 22, 2023 •

edited

Loading

albertnetymk Nov 22, 2023

tschatzl Nov 22, 2023

albertnetymk Nov 22, 2023

tschatzl commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

8320331: G1 Full GC Heap verification relies on metadata not reset before verification #16733

8320331: G1 Full GC Heap verification relies on metadata not reset before verification #16733

Conversation

tschatzl commented Nov 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 20, 2023

tschatzl commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

mlbridge bot commented Nov 20, 2023 • edited Loading

Webrevs

walulyai left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 22, 2023 • edited Loading

albertnetymk Nov 22, 2023

Choose a reason for hiding this comment

tschatzl Nov 22, 2023

Choose a reason for hiding this comment

albertnetymk Nov 22, 2023

Choose a reason for hiding this comment

tschatzl commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

tschatzl commented Nov 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 20, 2023 •

edited

Loading

openjdk bot commented Nov 22, 2023 •

edited

Loading