8361363: ShenandoahAsserts::print_obj() does not work for forwarded objects and UseCompactObjectHeaders

[tstuefe]

ShenandoahAsserts::print_obj() does not work for COH if the object is forwarded, since the mark word is overwritten with a forwarding pointer.

Depending on the size of the non-null bits of the forwardee (whether it spills into the nKlass bits), nKlass is either zero or garbage. So we get either an "assert(!is_null(v)) failed: narrow klass value can never be zero", or a SIGSEGV.

Fix is trivial: don't use obj->klass() directly but get it from ShenandoahForwarding.

---

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-8361363: ShenandoahAsserts::print_obj() does not work for forwarded objects and UseCompactObjectHeaders (Bug - P4)

Reviewers

• Roman Kennke (@rkennke - Reviewer)
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26117

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8361363: ShenandoahAsserts::print_obj() does not work for forwarded objects and UseCompactObjectHeaders

Reviewed-by: rkennke, shade

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

• 5c67e3d: 8361353: [PPC64] C2: Add nodes UMulHiL, CmpUL3, UMinV, UMaxV, NegVI
• 2349304: 8361040: compiler/codegen/TestRedundantLea.java#StringInflate fails with failed IR rules
• 0bd2f9c: 8361445: javac crashes on unresolvable constant in @SuppressWarnings
• ... and 45 more: https://git.openjdk.org/jdk/compare/5e40fb6bda1d56e3eba584b49aa0b68096b34169...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.

[openjdk]

@tstuefe The following labels will be automatically applied to this pull request:

• hotspot-gc
• shenandoah

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

[mlbridge]

Webrevs

• 00: Full (37fcc0f1)

[tstuefe]

Ping @shipilev, maybe? Or @rkennke? Its really trivial.

[rkennke]

Good! Thank you!

[shipilev]

There are two peculiarities in this method. It is normally called from diagnostics code, and so we are not 100% sure it it is even safe to reach through the fwdptr for klass. Second, we should be printing the raw header at least in some cases, because again, diagnostics.

Let me think about it...

[shipilev]

You know what, this is fine as it is.

The whole business with ShenandoahForwarding::klass is super-duper-awkward, and verifier already touches the forwardees using that method in places where it really should not. It needs a more comprehensive fix for +UCOH.

It is a mess, but not your mess. So integrate this one, we will figure out ShenandoahForwarding::klass after this.

[tstuefe]

You know what, this is fine as it is.

The whole business with ShenandoahForwarding::klass is super-duper-awkward, and verifier already touches the forwardees using that method in places where it really should not. It needs a more comprehensive fix for +UCOH.

It is a mess, but not your mess. So integrate this one, we will figure out ShenandoahForwarding::klass after this.

I had a similar thought while working on https://bugs.openjdk.org/browse/JDK-8361342 . ShenandoahForwarding::klass resolves the narrow Klass pointer, which may already assert if that one is null or invalid. I mentally left it for a rainy friday afternoon to fix.

And yes, printing the raw header (and possibly the first few payload words, to get a bit of the first members) I found very useful. os::print_hex_dump will tiptoe around invalid memory, so that should be fine to do.

[tstuefe]

Thanks @shipilev and @rkennke !

/integrate

[openjdk]

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

• e2c5d03: 8361526: Synchronize ClassFile API verifier with hotspot
• 19bb6eb: 8361484: Remove duplicate font filename mappings in fontconfig.properties for AIX
• 54e3762: 8361504: RISC-V: Make C1 clone intrinsic platform guard more specific
• ... and 59 more: https://git.openjdk.org/jdk/compare/5e40fb6bda1d56e3eba584b49aa0b68096b34169...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit fe26467.

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