8348631: Crash in PredictedCallGenerator::generate after JDK-8347006 #23331

TobiHartmann · 2025-01-28T13:10:37Z

We crash / assert during C2 compilation of intrinsics like _getLength because the cast emitted by the array guard added by JDK-8347006 is folded to top:

jdk/src/hotspot/share/opto/library_call.cpp

Lines 4302 to 4305 in c33c1cf

    
           if (obj != nullptr && is_array_ctrl != nullptr && is_array_ctrl != top()) { 
        
             // Keep track of the fact that 'obj' is an array to prevent 
        
             // array specific accesses from floating above the guard. 
        
             *obj = _gvn.transform(new CastPPNode(is_array_ctrl, *obj, TypeAryPtr::BOTTOM));

This happens when C2's type system determines that the type of the object that we cast implements an interface other than Serializable or Cloneable and therefore can't be an array. This is possible since JDK-8297933. Now unfortunately, control via the layout helper check is not (yet) folded due to:

jdk/src/hotspot/share/opto/memnode.cpp

Lines 2215 to 2223 in c33c1cf

    
           // If the type is enough to determine that the thing is not an array, 
        
           // we can give the layout_helper a positive interval type. 
        
           // This will help short-circuit some reflective code. 
        
           if (tkls->offset() == in_bytes(Klass::layout_helper_offset()) && 
        
               tkls->isa_instklassptr() && // not directly typed as an array 
        
               !tkls->is_instklassptr()->instance_klass()->is_java_lang_Object() // not the supertype of all T[] and specifically not Serializable & Cloneable 
        
               ) { 
        
             // Note:  When interfaces are reliable, we can narrow the interface 
        
             // test to (klass != Serializable && klass != Cloneable).

This is probably an oversight from JDK-8297933. Given that this is a regression in JDK 24, I'm going with a conservative approach of simply checking the cast for top and not using it if that's the case. In addition, I made the code more robust and added a compilation bailout (assert in debug) if an intrinsic produces a top result.

We should then properly fix this by making sure that the layout helper check is folded. I filed JDK-8348853 for this.

Big thanks to @cushon for reporting this just in time for fixing in JDK 24!

Best regards,
Tobias

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-8348631: Crash in PredictedCallGenerator::generate after JDK-8347006 (Bug - P2)(⚠️ The fixVersion in this issue is [24] but the fixVersion in .jcheck/conf is 25, a new backport will be created when this pr is integrated.)

Reviewers

Emanuel Peter (@eme64 - Reviewer) Review applies to 4cf7f864
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23331

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-28T13:11:34Z

👋 Welcome back thartmann! 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 · 2025-01-28T13:11:56Z

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

8348631: Crash in PredictedCallGenerator::generate after JDK-8347006

Reviewed-by: kvn, epeter

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

1efae9a: 8348888: tier1 closed build failure on Windows after JDK-8348348
c018a60: 8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows
c3c3888: 8336760: [JVMCI] -XX:+PrintCompilation should also print "hosted" JVMCI compilations
9f4d3de: 8347718: Unexpected NullPointerException in C2 compiled code due to ReduceAllocationMerges
a224f12: 8348205: Improve cutover time selection when building available currencies set
8103256: 8348348: Remove unnecessary #ifdef STATIC_BUILD around DEF_STATIC_JNI_OnLoad from zip_util.c
fb066ca: 8347272: [ubsan] JvmLauncher.cpp:262:52: runtime error: applying non-zero offset 40 to null pointer
5fec999: 8339668: Parallel: Adopt PartialArrayState to consolidate marking stack in Full GC

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2025-01-28T13:12:31Z

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

graal
hotspot-compiler

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.

eme64

Looks good.

Thanks for the offline explanation about how you found the failing bailout cases you need to catch: you just hard-coded the bailout for all such intrinsics, and ran testing. I think that should be sufficient for now.

mlbridge · 2025-01-28T15:40:26Z

Webrevs

01: Full - Incremental (54fd3894)
00: Full (4cf7f864)

vnkozlov

General question: how in other part of VM (runtime, gc) layout helper was changed for JDK-8297933?

vnkozlov · 2025-01-28T17:13:34Z

src/hotspot/share/opto/library_call.cpp

+    Node* cast = _gvn.transform(new CastPPNode(is_array_ctrl, *obj, TypeAryPtr::BOTTOM));
+    if (!cast->is_top()) {
+      *obj = cast;
+    }


Add comment why it could be TOP.

I added a comment.

cushon · 2025-01-28T17:40:24Z

I tested these changes against the original issue that prompted JDK-8348631, and everything looks good.

Thanks for the quick fix!

TobiHartmann · 2025-01-28T20:36:30Z

Thanks for the reviews Emanuel and Vladimir!

General question: how in other part of VM (runtime, gc) layout helper was changed for JDK-8297933?

@vnkozlov The layout helper was not changed but IIUC (@rwestrel, please correct me if I'm wrong), the type system has now enough information about interfaces that it can determine that casting an object implementing an interface to an array must be TOP. However, the layout helper check is not folded.

I tested these changes against the original issue that prompted JDK-8348631, and everything looks good.

@cushon Thanks for checking!

vnkozlov

Good

TobiHartmann · 2025-01-29T07:11:20Z

Thanks again, Vladimir!

TobiHartmann · 2025-01-29T07:12:24Z

/integrate

openjdk · 2025-01-29T07:13:13Z

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

98a93e1: 8348800: Many serviceability/sa tests failing after JDK-8348239
5e81fa6: 8348892: Properly fix compilation error for zip_util.c on Windows
3a564ed: 8347955: TimeZone methods to stream the available timezone IDs
1efae9a: 8348888: tier1 closed build failure on Windows after JDK-8348348
c018a60: 8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows
c3c3888: 8336760: [JVMCI] -XX:+PrintCompilation should also print "hosted" JVMCI compilations
9f4d3de: 8347718: Unexpected NullPointerException in C2 compiled code due to ReduceAllocationMerges
a224f12: 8348205: Improve cutover time selection when building available currencies set
8103256: 8348348: Remove unnecessary #ifdef STATIC_BUILD around DEF_STATIC_JNI_OnLoad from zip_util.c
fb066ca: 8347272: [ubsan] JvmLauncher.cpp:262:52: runtime error: applying non-zero offset 40 to null pointer
... and 1 more: https://git.openjdk.org/jdk/compare/2bef5b4a877f4d3bc766558b8782b7b57dee79a8...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-29T07:13:32Z

@TobiHartmann Pushed as commit 55c3e78.

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

TobiHartmann · 2025-01-29T07:15:23Z

/backport :jdk24

openjdk · 2025-01-29T07:16:02Z

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-55c3e78f-jdk24 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk24, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 55c3e78f from the openjdk/jdk repository.

The commit being backported was authored by Tobias Hartmann on 29 Jan 2025 and was reviewed by Vladimir Kozlov and Emanuel Peter.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-55c3e78f-jdk24:backport-TobiHartmann-55c3e78f-jdk24
$ git checkout backport-TobiHartmann-55c3e78f-jdk24
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-55c3e78f-jdk24

TobiHartmann added 2 commits January 28, 2025 13:04

8348631: Crash in PredictedCallGenerator::generate after JDK-8347006

f791c58

Fix

8bde5df

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jan 28, 2025

Trailing ws

4cf7f86

eme64 approved these changes Jan 28, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 28, 2025

vnkozlov reviewed Jan 28, 2025

View reviewed changes

Added comment

54fd389

openjdk bot removed the ready Pull request is ready to be integrated label Jan 28, 2025

vnkozlov approved these changes Jan 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 28, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 29, 2025

openjdk bot closed this Jan 29, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 29, 2025

marc-chevalier mentioned this pull request Mar 26, 2025

8348853: Fold layout helper check for objects implementing non-array interfaces #24245

Closed

3 tasks

	if (obj != nullptr && is_array_ctrl != nullptr && is_array_ctrl != top()) {
	// Keep track of the fact that 'obj' is an array to prevent
	// array specific accesses from floating above the guard.
	obj = _gvn.transform(new CastPPNode(is_array_ctrl, obj, TypeAryPtr::BOTTOM));

	// If the type is enough to determine that the thing is not an array,
	// we can give the layout_helper a positive interval type.
	// This will help short-circuit some reflective code.
	if (tkls->offset() == in_bytes(Klass::layout_helper_offset()) &&
	tkls->isa_instklassptr() && // not directly typed as an array
	!tkls->is_instklassptr()->instance_klass()->is_java_lang_Object() // not the supertype of all T[] and specifically not Serializable & Cloneable
	) {
	// Note: When interfaces are reliable, we can narrow the interface
	// test to (klass != Serializable && klass != Cloneable).

8348631: Crash in PredictedCallGenerator::generate after JDK-8347006 #23331

8348631: Crash in PredictedCallGenerator::generate after JDK-8347006 #23331

Uh oh!

Conversation

TobiHartmann commented Jan 28, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 28, 2025

Uh oh!

openjdk bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 28, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

cushon commented Jan 28, 2025

Uh oh!

TobiHartmann commented Jan 28, 2025

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 29, 2025

Uh oh!

TobiHartmann commented Jan 29, 2025

Uh oh!

openjdk bot commented Jan 29, 2025

Uh oh!

openjdk bot commented Jan 29, 2025

Uh oh!

TobiHartmann commented Jan 29, 2025

Uh oh!

openjdk bot commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

TobiHartmann commented Jan 28, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 28, 2025 •

edited

Loading

mlbridge bot commented Jan 28, 2025 •

edited

Loading