8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap #12456

tschatzl · 2023-02-07T15:03:36Z

Hi all,

can I have reviews for this change to liveness verification that fixes some unwanted asserts because

it uses decode_not_null which will assert if the given oop address is not in the heap, making the remainder of the verification useless in that case
if the referenced object is not in the heap, we try to get its heap region too when printing, which also fails some assertions
in the innermost if lots of code is duplicated in both cases

The first two issues are really annoying (there is another one when the Klass is garbage when calling is_obj_dead_cond, but I'll try to improve that separately).

Testing: local compilation/testing, gha

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-8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to b2c41af7
Albert Mingkun Yang (@albertnetymk - Reviewer) ⚠️ Review applies to b8182317

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12456

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

Using diff file

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

bridgekeeper · 2023-02-07T15:04:30Z

👋 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.

openjdk · 2023-02-07T15:06:46Z

@tschatzl The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-02-07T15:10:20Z

Webrevs

dholmes-ora

Mostly seems quite reasonable based on the description, but one query below.

Thanks.

dholmes-ora · 2023-02-08T02:26:26Z

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

@@ -214,7 +214,7 @@ inline bool G1CollectedHeap::requires_barriers(stackChunkOop obj) const {
 }

 inline bool G1CollectedHeap::is_obj_filler(const oop obj) {
-  Klass* k = obj->klass();
+  Klass* k = obj->klass_raw();


Not clear how you can get here from HeapRegion::is_obj_dead with a bad oop, such that you need the raw variant. ??

The object is in the heap, but the occupying memory has already been zapped (in debug mode); i.e. the call in heapRegion.cpp:518 could read badHeapWordVal as (compressed) klass value in the header.

In that case the current code asserts in this call because in oopDesc::klass(), the call to CompressedKlassPointers::decode_not_null will assert in compressedOops.inline.hpp:135 due to the check_alignment condition not satisfied.

This makes this verification code assert before printing out any useful information to diagnose the problem quickly (in my case this has been a change that wrongly managed remembered sets).

If I had had the remembered set verification printout, I would have found the problem immediately in this case (because the message would have told me that there is a problem with remembered sets). So it took a while to diagnose the issue, having to go into the debugger to painfully find the exact same information.

I.e. this makes the verification code more robust.

Imo the suggested solution to just continue execution is fine, because is_obj_filler will always return false (i.e. object is dead) for garbage objects and do the right thing here.
There is the concern that now other non-verification code might not immediately trigger now, but most of it just fails the VM anyway if it finds a bad reference (after printing some information about it), for all other cases this is the right choice.

Thanks for the explanation.

dholmes-ora

LGTM. Thanks

openjdk · 2023-02-09T03:34:58Z

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

8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap

Reviewed-by: dholmes, 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 38 new commits pushed to the master branch:

597a9a4: 8301822: BasicLookAndFeel does not need to check for null after checking for type
3b05a94: 8301858: Verification error when compiling switch with record patterns
e4d1cff: 8300268: ServerImpl allows too many idle connections when using sun.net.httpserver.maxIdleConnections
af8973d: Merge
e81f20b: 8301989: new javax.swing.text.DefaultCaret().setBlinkRate(N) results in NPE
6f460e4: 8301863: ObjectInputFilter example incorrectly calls rejectUndecidedClass
2caa56a: 8301843: Remove dummy region allocation
d401982: 8302121: Parallel: Remove unused arg in PSCardTable::inline_write_ref_field_gc
916374d: 8302067: [AIX] AIX build error on os_aix_ppc.cpp
a4039e3: 8302043: [AIX] Safefetch fails for bad_addressN and bad_address32
... and 28 more: https://git.openjdk.org/jdk/compare/4fe99da74f557461c31293cdc48af1199dd2b85c...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.

tschatzl · 2023-02-09T16:35:55Z

/label add hotspot-gc

openjdk · 2023-02-09T16:36:37Z

@tschatzl
The hotspot-gc label was successfully added.

albertnetymk · 2023-02-09T17:36:10Z

src/hotspot/share/gc/g1/heapRegion.cpp

-          print_object(&ls, obj);
-        }
+    oop obj = CompressedOops::decode_raw_not_null(heap_oop);
+    bool failed = false;


tschatzl · 2023-02-09T20:07:22Z

Thanks @dholmes-ora @albertnetymk for your reviews

/integrate

openjdk · 2023-02-09T20:09:23Z

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

4815566: 8228604: StackMapFrames are missing from redefined class bytes of retransformed classes
5147969: 8272288: Funky multiresolution image breaks graphics context
77ead44: 8302066: Counter _number_of_nmethods_with_dependencies should be atomic.
f4b72df: 8282379: [LOOM] vmTestbase/nsk/jdi/ClassType/invokeMethod/invokemethod011 sometimes fails
7fd440d: 8298868: Update EngineCloseOnAlert.java for changes to TLS implementation
7901f45: 8301260: Add system property to toggle XML Signature secure validation mode
597a9a4: 8301822: BasicLookAndFeel does not need to check for null after checking for type
3b05a94: 8301858: Verification error when compiling switch with record patterns
e4d1cff: 8300268: ServerImpl allows too many idle connections when using sun.net.httpserver.maxIdleConnections
af8973d: Merge
... and 34 more: https://git.openjdk.org/jdk/compare/4fe99da74f557461c31293cdc48af1199dd2b85c...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-09T20:09:33Z

@tschatzl Pushed as commit 0aeebee.

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

initial version

3066f5f

openjdk bot changed the title ~~8301988~~ 8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap Feb 7, 2023

openjdk bot added the rfr Pull request is ready for review label Feb 7, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 7, 2023

Missing changes

b2c41af

dholmes-ora reviewed Feb 8, 2023

View reviewed changes

dholmes-ora approved these changes Feb 9, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2023

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Feb 9, 2023

remove one level of indentation in do_oop_work

b818231

albertnetymk approved these changes Feb 9, 2023

View reviewed changes

removed never-read failed local variable. Formatting cleanup.

96523f9

openjdk bot added the integrated Pull request has been integrated label Feb 9, 2023

openjdk bot closed this Feb 9, 2023

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

tschatzl deleted the submit/8301988-verifyliveness-bad-pointers branch February 20, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap #12456

8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap #12456

tschatzl commented Feb 7, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

mlbridge bot commented Feb 7, 2023 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Feb 8, 2023

tschatzl Feb 8, 2023

dholmes-ora Feb 9, 2023

dholmes-ora left a comment

openjdk bot commented Feb 9, 2023 •

edited

Loading

tschatzl commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

albertnetymk Feb 9, 2023

tschatzl commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap #12456

8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap #12456

Conversation

tschatzl commented Feb 7, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

mlbridge bot commented Feb 7, 2023 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Feb 8, 2023

Choose a reason for hiding this comment

tschatzl Feb 8, 2023

Choose a reason for hiding this comment

dholmes-ora Feb 9, 2023

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 9, 2023 • edited Loading

tschatzl commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

albertnetymk Feb 9, 2023

Choose a reason for hiding this comment

tschatzl commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

tschatzl commented Feb 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2023 •

edited

Loading

openjdk bot commented Feb 9, 2023 •

edited

Loading