8371216: oopDesc::print_value_on breaks if klass is garbage

[Arraying]

Hi all,

The oopDesc::print_value_on function checks if an oop is a string, and if so just prints the raw string. To do this, it needs to read the klass(). If the klass() reads garbage, one of many assertion errors is likely triggered.

For example, if G1's verification finds problematic oops, it will attempt to print them. If these oops have garbage (incorrect or racey) klasses, this will cause an assertion error, fail fast, and VM crash. G1 never finishes printing, which may make debugging more difficult. The developer can/will be made aware in other ways if the klass() is garbage, for example by being told that it is not in the metaspace.

We observed the above in Valhalla and already patched it there.

Testing: tiers 1-5 on Linux (x64, AArch64), macOS (x64, AArch64), Windows (x64).

---

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-8371216: oopDesc::print_value_on breaks if klass is garbage (Bug - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28190

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back phubner! 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]

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

8371216: oopDesc::print_value_on breaks if klass is garbage

Reviewed-by: coleenp, mdoerr

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

• c865644: 8365932: Implementation of JEP 516: Ahead-of-Time Object Caching with Any GC
• d5803aa: 8371315: java/foreign/sharedclosejfr/TestSharedCloseJFR.java failed with -XX:-TieredCompilation
• 167c952: 8371369: Parallel: Relax precondition of PSOldGen::expand_and_allocate
• ... and 25 more: https://git.openjdk.org/jdk/compare/1b3889a47092e018ab9ecb6aaa922046d8d0e916...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tstuefe, @coleenp, @TheRealMDoerr) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@Arraying 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]

Webrevs

• 01: Full - Incremental (b5991b7f)
• 00: Full (e8be53d0)

[tstuefe]

I would not add the helper function for only one case. We already have a bunch of helpers like these. Just test for oop != NULL at the call site.

[Arraying]

I would not add the helper function for only one case. We already have a bunch of helpers like these. Just test for oop != NULL at the call site.

In the scenario I am referring to, we had an always-non-null oop and went through CompressedKlassPointers::decode_not_null (which has like 12 assertions). Most of the time, we failed because of the klass range check. In rare cases for this particular instance, but quite often in often in other cases like JDK-8366794, the klass was null. I don't think it's sufficient to just check for oop nullness.

I agree with you, it'd be nicer to check at the call site. How do you feel about inlining the proposed function into:

  if (obj != nullptr && obj->klass_without_asserts() == vmClasses::String_klass()) {
    java_lang_String::print(obj, st);
    print_address_on(st);
  } else // [...]

[coleenp]

I like the inlining option.

[coleenp]

This looks good!

[TheRealMDoerr]

Thanks for fixing it!
I think there are more places which cause "error occurred during error reporting" due to assertions, but they can get addressed in separate issues.

[Arraying]

Thanks for the feedback and reviews @tstuefe @coleenp @TheRealMDoerr. I'll integrate this on Monday when the 24h period has passed and my test re-runs are finished.

I think there are more places which cause "error occurred during error reporting" due to assertions, but they can get addressed in separate issues.

If you have any examples, either now or down the road, I'd be happy to take a look.

[TheRealMDoerr]

Thanks for the feedback and reviews @tstuefe @coleenp @TheRealMDoerr. I'll integrate this on Monday when the 24h period has passed and my test re-runs are finished.

I think there are more places which cause "error occurred during error reporting" due to assertions, but they can get addressed in separate issues.

If you have any examples, either now or down the road, I'd be happy to take a look.

An prominent example in the debug build is the following assertion: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/compressedKlass.inline.hpp#L91
The code is used by CompressedKlassPointers::decode_not_null. The error reporting code shouldn't use the version with assertions while investigating if a value may be a compressed class pointer.