8347006: LoadRangeNode floats above array guard in arraycopy intrinsic #22967

TobiHartmann · 2025-01-08T12:07:16Z

C2's arraycopy intrinsic adds guards that check that the source and destination objects are arrays:

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

Lines 5917 to 5919 in afe5434

    
           // (1) src and dest are arrays. 
        
           generate_non_array_guard(load_object_klass(src), slow_region); 
        
           generate_non_array_guard(load_object_klass(dest), slow_region);

If these guards pass, the array length is loaded:

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

Lines 5930 to 5933 in afe5434

    
           // (7) src_offset + length must not exceed length of src. 
        
           generate_limit_guard(src_offset, length, 
        
                                load_array_length(src), 
        
                                slow_region);

But since the LoadRangeNode is not pinned, it might float above the array guard:

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

Line 1214 in afe5434

    
           alen = _gvn.transform( new LoadRangeNode(nullptr, immutable_memory(), r_adr, TypeInt::POS));

If the object is not an array, we will read garbage. That's usually fine because the result will not be used (the array guard will trigger) but with -XX:+UseCompactObjectHeaders it can happen that the memory right after the header is not mapped and we crash.

The fix is to add a CheckCastPPNode to propagate the information that the operand is an array and prevent the load from floating.

Thanks to @shipilev for identifying the root cause!

I was able to reliably reproduce the issue with compiler/arraycopy/TestArrayCopyNoInit.java and -XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders on Linux AArch64 and verified that the fix solves the problem.

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-8347006: LoadRangeNode floats above array guard in arraycopy intrinsic (Bug - P3)(⚠️ 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

Roland Westrelin (@rwestrel - Reviewer)
Quan Anh Mai (@merykitty - Committer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22967

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-08T12:07:57Z

👋 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-08T12:08:22Z

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

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic

Reviewed-by: roland, qamai, kvn

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

ed0b555: 8344035: Replace predicate walking code in Loop Unswitching with a predicate visitor
b37f123: 8347407: [BACKOUT] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
1f7925c: 8347270: Remove unix_getParentPidAndTimings, unix_getChildren and unix_getCmdlineAndUserInfo
9fafd63: 8346828: javax/swing/JScrollBar/4865918/bug4865918.java still fails in CI
f04a642: 8346717: serviceability/dcmd/vm/SystemDumpMapTest.java failing on Windows with "Stack base not yet set for thread id"
3145278: 8346727: JvmtiVTMSTransitionDisabler deadlock
761774a: 8346143: add ClearAllFramePops function to speedup debugger single stepping in some cases
6f1f2f2: 8347063: Add comments in ClassFileFormatVersion for class file format evolution history
10f7142: 8347295: Fix WinResourceTest to make it work with WiX v4.0+
01c8ba2: 8347298: Bug in JPackageCommand.ignoreFakeRuntime()
... and 67 more: https://git.openjdk.org/jdk/compare/bcefab5e55d4527a38dcab550581a734c1564608...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 · 2025-01-08T12:08:59Z

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

mlbridge · 2025-01-08T12:12:31Z

Webrevs

rwestrel

Looks good to me.

rwestrel · 2025-01-08T12:17:05Z

src/hotspot/share/opto/library_call.cpp

    generate_non_array_guard(load_object_klass(src), slow_region);
+    const Type* tary = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::BOTTOM, TypeInt::POS), nullptr, false, Type::OffsetBot);


Is this never used elsewhere? Should it a static field in TypeAryPtr same as TypeAryPtr::BYTES and friends?

I wondered as well and no, we don't use this type anywhere else (the closest would be TypeAryPtr::RANGE). We only create it when meeting arrays of primitive and non-primitive element type. Do you think this should still go to TypeAryPtr::*?

I would add it to TypeAryPtr (maybe as TypeAryPtr::BOTTOM) . The main benefit I see is that the new code would be more readable if it referred to TypeAryPtr::BOTTOM rather than the long type creation expression.

Sounds good, I'll update the patch accordingly.

shipilev

I have a question about the test :)

shipilev · 2025-01-08T12:20:24Z

test/hotspot/jtreg/compiler/arraycopy/TestArrayCopyNoInit.java

 * @summary Deoptimization between array allocation and arraycopy may result in non initialized array
 *
 * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
 *                   compiler.arraycopy.TestArrayCopyNoInit
+ * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
+ *                   -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders -XX:-UseTLAB


You have been able to reproduce this with -UseCompressedClassPointers, right? If so, I'd suggest we do a run config with -UseCCP instead of +UseCOH, because this gives us a cleaner way for backports, if we need one later.

You have been able to reproduce this with -UseCompressedClassPointers, right?

No, I was never able to reproduce this with -XX:-UseCompressedClassPointers.

OK, I was confused by this in PR body then:

I was able to reliably reproduce the issue with compiler/arraycopy/TestArrayCopyNoInit.java and -XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:-UseCompressedClassPointers on Linux AArch64 and verified that the fix solves the problem.

But fine, if it reproduces with +UCOH, let it be there.

Ah, that's actually a typo, good catch. Should be -XX:+UseCompactObjectHeaders. I'll fix it in the description.

Cant you please add this '@run' as a separate testcase with it's own id. So it is easier to identify and exclude the failures.

TobiHartmann · 2025-01-08T12:23:14Z

Thanks for the review, Roland!

shipilev · 2025-01-08T12:50:14Z

I think bug should target 25 and then we backport it to 24.

TobiHartmann · 2025-01-08T13:03:11Z

I think bug should target 25 and then we backport it to 24.

Since we should fix the issue in JDK 24, the bug should remain targeted to JDK 24. The Skara bot will then take care of updating the fix version to JDK 25 and creating a backport to JDK 24 once we push this into master.

TobiHartmann · 2025-01-08T13:05:28Z

I updated the fix according to Roland's suggestions and also added missing stopped checks (otherwise, the Cast will become TOP and below code does not like that).

rwestrel · 2025-01-08T13:10:51Z

src/hotspot/share/opto/library_call.cpp

    generate_non_array_guard(load_object_klass(src), slow_region);
+    if (!stopped()) {


Shouldn't we simply return then?

But we need to set up the slow_region path, right?

But we need to set up the slow_region path, right?

By that you mean have the slow_region feed into the uncommon trap that's only created later. It does feel weird that we know we have reached a dead end and we keep trying to add stuff, but ok then.
The other thing is shouldn't the cast be added in generate_non_array_guard()? I see it's used elsewhere (LibraryCallKit::inline_native_getLength()): couldn't the same bug occur there?

By that you mean have the slow_region feed into the uncommon trap that's only created later.

Right. It's a bit weird but probably still the best solution in terms of complexity. The trap will lead to recompilation and then the too_many_traps check will trigger.

The other thing is shouldn't the cast be added in generate_non_array_guard()? I see it's used elsewhere (LibraryCallKit::inline_native_getLength()): couldn't the same bug occur there?

Right, good catch. I think the use in LibraryCallKit::inline_native_getLength has the same problem. We can't easily put the cast into generate_non_array_guard though because it operates on the Klass and not on the object. The other generate*array*guard methods potentially have the same issue but current uses look good. I guess it's best to fix the LibraryCallKit::inline_native_getLength as well, i.e., make it the caller's responsibility to add a cast. What do you think?

Hmm, maybe inline_getObjectSize is affected as well:

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

Lines 8535 to 8543 in afe5434

Node* array_ctl = generate_array_guard(klass_node, nullptr);

if (array_ctl != nullptr) {

// Array case: size is round(header + element_size*arraylength).

// Since arraylength is different for every array instance, we have to

// compute the whole thing at runtime.

PreserveJVMState pjvms(this);

set_control(array_ctl);

Node* arr_length = load_array_length(obj);

And LibraryCallKit::inline_native_clone as well:

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

Lines 5257 to 5262 in afe5434

Node* array_ctl = generate_array_guard(obj_klass, (RegionNode*)nullptr);

if (array_ctl != nullptr) {

// It's an array.

PreserveJVMState pjvms(this);

set_control(array_ctl);

Node* obj_length = load_array_length(obj);

I guess it's best to fix the LibraryCallKit::inline_native_getLength as well, i.e., make it the caller's responsibility to add a cast. What do you think?

Maybe the methods need to take an extra parameter (the object to cast)?
Having the cast in the method would lead to less code duplication and a lower risk of forgetting the cast when new calls of the method are added so that's what I would go with unless it's really a pain.

Right, I'll give that a try.

merykitty · 2025-01-08T16:09:28Z

src/hotspot/share/opto/library_call.cpp

    generate_non_array_guard(load_object_klass(src), slow_region);
+    if (!stopped()) {
+      src = _gvn.transform(new CheckCastPPNode(control(), src, TypeAryPtr::BOTTOM));


Why is this a CheckCastPP and not a CastPP? My understanding is that a CheckCastPP is used when we force changing the type of a node (e.g a raw pointer of Allocate into a typed pointer), so we do not join the type of the input with that of the output.

Good point, I changed that.

merykitty · 2025-01-08T16:12:56Z

src/hotspot/share/opto/type.hpp

@@ -1473,6 +1473,7 @@ class TypeAryPtr : public TypeOopPtr {
  virtual const TypeKlassPtr* as_klass_type(bool try_for_exact = false) const;

  // Convenience common pre-built types.
+  static const TypeAryPtr* BOTTOM;


While you are here it may be better to change the other constant to TypeAryPtr* instead of TypeAryPtr *

Right, done.

TobiHartmann · 2025-01-09T10:17:54Z

Roland, Quan Anh, thanks for the reviews! I pushed a new version that should address all comments.

TobiHartmann · 2025-01-09T13:21:03Z

Thanks again for the review, @merykitty!

vnkozlov · 2025-01-09T16:28:46Z

src/hotspot/share/opto/library_call.cpp

+  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));


Should we do this for above code when layout is known for compiler (layout_con is checked)?

I thought about this as well but I don't think it's necessary because:

No cast is needed if we know the type already

We don't emit a guard and only one branch remains, so there is no risk of the array specific access floating above

So I went with the simplest changes for now, also since we need to backport this change (it got already much more complicated than I was aiming for).

vnkozlov

Good.

TobiHartmann · 2025-01-13T06:09:07Z

Thanks for the review, Vladimir! @rwestrel are you okay with the changes as well?

rwestrel

Looks good to me.

TobiHartmann · 2025-01-13T09:48:14Z

Thanks again, Roland!

TobiHartmann · 2025-01-13T09:48:19Z

/integrate

openjdk · 2025-01-13T09:49:18Z

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

85ed78c: 8345185: Update jpackage to not include service bindings by default
3b9732e: 8345471: Clean up compiler/intrinsics/sha/cli tests
ed0b555: 8344035: Replace predicate walking code in Loop Unswitching with a predicate visitor
b37f123: 8347407: [BACKOUT] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
1f7925c: 8347270: Remove unix_getParentPidAndTimings, unix_getChildren and unix_getCmdlineAndUserInfo
9fafd63: 8346828: javax/swing/JScrollBar/4865918/bug4865918.java still fails in CI
f04a642: 8346717: serviceability/dcmd/vm/SystemDumpMapTest.java failing on Windows with "Stack base not yet set for thread id"
3145278: 8346727: JvmtiVTMSTransitionDisabler deadlock
761774a: 8346143: add ClearAllFramePops function to speedup debugger single stepping in some cases
6f1f2f2: 8347063: Add comments in ClassFileFormatVersion for class file format evolution history
... and 69 more: https://git.openjdk.org/jdk/compare/bcefab5e55d4527a38dcab550581a734c1564608...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-13T09:49:31Z

@TobiHartmann Pushed as commit 82e2a79.

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

TobiHartmann · 2025-01-13T09:55:21Z

/backport :jdk24

openjdk · 2025-01-13T09:56:16Z

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-82e2a791-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 82e2a791 from the openjdk/jdk repository.

The commit being backported was authored by Tobias Hartmann on 13 Jan 2025 and was reviewed by Roland Westrelin, Quan Anh Mai and Vladimir Kozlov.

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-82e2a791-jdk24:backport-TobiHartmann-82e2a791-jdk24
$ git checkout backport-TobiHartmann-82e2a791-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-82e2a791-jdk24

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic

425bbb6

openjdk bot added the rfr Pull request is ready for review label Jan 8, 2025

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

rwestrel approved these changes Jan 8, 2025

View reviewed changes

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

shipilev reviewed Jan 8, 2025

View reviewed changes

Added missing stopped checks, refactoring and updated copyright dates

5c0292a

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

rwestrel reviewed Jan 8, 2025

View reviewed changes

merykitty reviewed Jan 8, 2025

View reviewed changes

Moved cast into guard

3b465a4

Copyright date

0a1fe38

merykitty approved these changes Jan 9, 2025

View reviewed changes

vnkozlov reviewed Jan 9, 2025

View reviewed changes

vnkozlov approved these changes Jan 10, 2025

View reviewed changes

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

rwestrel approved these changes Jan 13, 2025

View reviewed changes

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

openjdk bot closed this Jan 13, 2025

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

	// (1) src and dest are arrays.
	generate_non_array_guard(load_object_klass(src), slow_region);
	generate_non_array_guard(load_object_klass(dest), slow_region);

	// (7) src_offset + length must not exceed length of src.
	generate_limit_guard(src_offset, length,
	load_array_length(src),
	slow_region);

		generate_non_array_guard(load_object_klass(src), slow_region);
		const Type* tary = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::BOTTOM, TypeInt::POS), nullptr, false, Type::OffsetBot);

		generate_non_array_guard(load_object_klass(src), slow_region);
		if (!stopped()) {

	Node* array_ctl = generate_array_guard(klass_node, nullptr);
	if (array_ctl != nullptr) {
	// Array case: size is round(header + element_size*arraylength).
	// Since arraylength is different for every array instance, we have to
	// compute the whole thing at runtime.

	PreserveJVMState pjvms(this);
	set_control(array_ctl);
	Node* arr_length = load_array_length(obj);

	Node* array_ctl = generate_array_guard(obj_klass, (RegionNode*)nullptr);
	if (array_ctl != nullptr) {
	// It's an array.
	PreserveJVMState pjvms(this);
	set_control(array_ctl);
	Node* obj_length = load_array_length(obj);

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic #22967

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic #22967

Conversation

TobiHartmann commented Jan 8, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 8, 2025

openjdk bot commented Jan 8, 2025 • edited Loading

openjdk bot commented Jan 8, 2025

mlbridge bot commented Jan 8, 2025 • edited Loading

Webrevs

rwestrel left a comment

Choose a reason for hiding this comment

rwestrel Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwestrel Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann commented Jan 8, 2025

shipilev commented Jan 8, 2025

TobiHartmann commented Jan 8, 2025

TobiHartmann commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

TobiHartmann Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann commented Jan 9, 2025

TobiHartmann commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

TobiHartmann commented Jan 13, 2025

rwestrel left a comment

Choose a reason for hiding this comment

TobiHartmann commented Jan 13, 2025

TobiHartmann commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

TobiHartmann commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

TobiHartmann commented Jan 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 8, 2025 •

edited

Loading

mlbridge bot commented Jan 8, 2025 •

edited

Loading

rwestrel Jan 8, 2025 •

edited

Loading

rwestrel Jan 8, 2025 •

edited

Loading

TobiHartmann Jan 8, 2025 •

edited

Loading

TobiHartmann Jan 8, 2025 •

edited

Loading

TobiHartmann Jan 8, 2025 •

edited

Loading