8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access #20033

tobiasholenstein · 2024-07-04T12:17:51Z

We failed in LibraryCallKit::inline_unsafe_access() while trying to inline Unsafe::getShortUnaligned.

jdk/test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithNullBase.java

Line 86 in 34c6e0d

    
           return run ? UNSAFE.getShortUnaligned(array, 1) : 0; // after warmup CheckCastPP: speculative=byte[int:>=0]

The reason is that base (the array) is ConP #null hidden behind two CheckCastPP with speculative=byte[int:>=0]

We call Node* adr = make_unsafe_address(base, offset, type, kind == Relaxed);

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

Line 2361 in 34c6e0d

Node* adr = make_unsafe_address(base, offset, type, kind == Relaxed);

with base = 147 CheckCastPP
118 ConP === 0 [[[ 106 101 71 ] #null

Depending on the offset we go two different paths in LibraryCallKit::make_unsafe_address which both lead to the same error in the end.

For UNSAFE.getShortUnaligned(array, 1_049_000) we get kind = Type::AnyPtr because offset >= os::vm_page_size(). Since we assume base can't be null we insert an assert:

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

Line 2111 in 34c6e0d

base = must_be_not_null(base, true);
whereas for UNSAFE.getShortUnaligned(array, 1) we get kind = Type:: OopPtr

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

Line 2078 in c17fa91

int kind = classify_unsafe_addr(uncasted_base, offset, type);

and insert a null check

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

Line 2090 in 34c6e0d

base = null_check_oop(base, &null_ctl, true, true, true);

In both cases we return call basic_plus_adr(..) on a base being top() which returns adr = 1 Con === 0 [[ ]] #top

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

Line 2386 in 3d5d51e

const TypePtr* adr_type = _gvn.type(adr)->isa_ptr();

=> _gvn.type(adr) is top

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

Line 2394 in 3d5d51e

Compile::AliasType* alias_type = C->alias_type(adr_type);

=> adr_type is nullptr

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

Lines 2405 to 2406 in 3d5d51e

    
           BasicType bt = alias_type->basic_type(); 
        
           if (bt != T_ILLEGAL) {

=> BasicType bt is T_ILLEGAL

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

Line 2424 in 3d5d51e

} else if (alias_type->adr_type()->isa_oopptr()) {

=> we fail here with SIGSEGV: null pointer dereference because alias_type->adr_type() is nullptr

Fix (updated on 18th Sep 2024)

The fix modifies the LibraryCallKit::classify_unsafe_addr() method to handle cases where the base might be hidden behind speculative type information.

In the original situation, null_check_oop() detects that the base value is NULL and transforms the check into an unconditional uncommon trap. The problem arises because LibraryCallKit::classify_unsafe_addr() sometimes fails to recognize this due to speculative typing, particularly when the base is hidden behind CheckCastPP nodes.

To resolve this, we add base->uncast() in classify_unsafe_addr(). The uncast() method strips away the speculative information from the base node, allowing the comparison against TypePtr::NULL_PTR to succeed. This ensures that when the base is NULL, the method can properly classify the address and avoid generating dead or incorrect code.

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

Line 2044 in 2ab02f8

} else if (_gvn.type(base->uncast()) == TypePtr::NULL_PTR) {

In summary, the uncast() fix helps the method recognize when the base is NULL, aligning LibraryCallKit::classify_unsafe_addr() with the behavior of null_check_oop(), preventing errors when inlining unsafe accesses.

Testing: tier1-4 pass

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-8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access (Bug - P3)

Reviewers

Emanuel Peter (@eme64 - Reviewer) Review applies to 2abec7d4
Roland Westrelin (@rwestrel - Reviewer) Review applies to efcd729e
Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)
Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20033

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

Using diff file

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

Webrev

Link to Webrev Comment

…access

bridgekeeper · 2024-07-04T12:18:40Z

👋 Welcome back tholenstein! 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 · 2024-07-04T12:19:11Z

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

8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access

Reviewed-by: thartmann, kvn, vlivanov, epeter, roland

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

83dcb02: 8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes
d2e7708: 8341367: Problemlist ShapeNotSetSometimes.java on macOS
0314973: 8341060: Cleanup statics in HeapDumper
021bf63: 8340458: Open source additional Component tests (part 2)
9a7817b: 8340988: Update jdk/jfr/event/gc/collection tests to accept "CodeCache GC Threshold" as valid GC reason
f2a767f: 8340907: Open source closed frame tests # 2
7b1e6f8: 8337389: Parallel: Remove unnecessary forward declarations in psScavenge.hpp
2120a84: 8341333: [JVMCI] Export JavaThread::_unlocked_inflated_monitor to JVMCI
684d246: 8341242: Shenandoah: LRB node is not matched as GC barrier after JDK-8340183
7cc7c08: 8337493: [JVMCI] Number of libgraal threads might be too low
... and 94 more: https://git.openjdk.org/jdk/compare/279086d4ce7e05972e099022e8045f39680dd4e8...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 · 2024-07-04T12:19:40Z

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

remove UnlockDiagnosticVMOptions

…K-8320308

openjdk · 2024-07-08T13:39:48Z

⚠️ @tobiasholenstein This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

tobiasholenstein · 2024-07-09T16:44:30Z

/label remove graal

openjdk · 2024-07-09T16:45:08Z

@tobiasholenstein
The graal label was successfully removed.

mlbridge · 2024-07-09T18:03:42Z

Webrevs

vnkozlov · 2024-07-09T18:35:40Z

To understand what happens here. Did null check If control flow in make_unsafe_address() collapse and have only path to uncommon trap? This is what stopped() checks. In such case why it collapsed if NULL is hidden by CheckCastPP?

iwanowww · 2024-07-09T21:23:31Z

Even though the proposed check in LibraryCallKit::inline_unsafe_access() fixes the crash, IMO the root problem is in LibraryCallKit::classify_unsafe_addr() where base_type == TypePtr::NULL_PTR doesn't hold in presence of speculative part and the base is erroneously classified as on-heap (Type::OopPtr).

I'd prefer to see both places fixed. Seeing make_unsafe_address producing dead code signals about a bug. So, asserting that it never happens looks like a good idea.

Moreover, there are many places in the code susceptible to the same problem where a type is compared with TypePtr::NULL_PTR.

tobiasholenstein · 2024-07-10T12:37:10Z

To understand what happens here. Did null check If control flow in make_unsafe_address() collapse and have only path to uncommon trap? This is what stopped() checks.

Yes, exactly

In such case why it collapsed if NULL is hidden by CheckCastPP?

While casting the base to not null (null_check_oop(..)) we insert 150 CmpP. The following stack-trace of _gvn.transform(chk) to Node::eqv_uncastdetermines that after stripping casting the two nodes are equivalent

Node::eqv_uncast(n, keep_deps=false) at node.hpp:500
SubNode::Value_common(phase) at subnode.cpp:91
SubNode::Value(phase) at subnode.cpp:101
PhaseGVN::transform(n) at phaseX.cpp:703
GraphKit::null_check_common(value, type, assert_null, null_control, speculative) at graphKit.cpp:1316
GraphKit::null_check_oop(value, null_control, never_see_null, safe_for_replace, speculative) at graphKit.cpp:2455
LibraryCallKit::make_unsafe_address(base, offset, type, can_cast) at library_call.cpp:2090
LibraryCallKit::inline_unsafe_access(is_store, type, kind, unaligned) at library_call.cpp:2361

eme64

Looks good :)

TobiHartmann

Looks good to me too.

So it would make sense to go over the uses of Type*::BOTTOM/Type*::NOTNULL and check they are not tested with pointer equality

What about this concern? Did anyone check yet or should be file a follow-up task?

test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithNullBase.java

…ullBase.java Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithNullBase.java

vnkozlov · 2024-09-25T17:11:14Z

src/hotspot/share/opto/library_call.cpp

@@ -2362,6 +2362,7 @@ bool LibraryCallKit::inline_unsafe_access(bool is_store, const BasicType type, c
  SafePointNode* old_map = clone_map();

  Node* adr = make_unsafe_address(base, offset, type, kind == Relaxed);
+  assert(!stopped(), "Inlining of unsafe access failed: address construction stopped unexpectedly");

  if (_gvn.type(base)->isa_ptr() == TypePtr::NULL_PTR) {


Why not uncast here?

makes sense to add here as well. Done

tobiasholenstein · 2024-09-26T11:54:50Z

Looks good to me too.

So it would make sense to go over the uses of Type*::BOTTOM/Type*::NOTNULL and check they are not tested with pointer equality

What about this concern? Did anyone check yet or should be file a follow-up task?

I filed a follow up task: https://bugs.openjdk.org/browse/JDK-8341023

vnkozlov

Good.

tobiasholenstein · 2024-09-27T13:11:30Z

Thanks @TobiHartmann , @rwestrel , @eme64, @vnkozlov and @iwanowww for the reviews!

If @iwanowww is ok with the changes, this PR is ready to integrate.

I will delegate since I am out of office the next 3 weeks.

/integrate delegate

openjdk · 2024-09-27T13:12:59Z

@tobiasholenstein Integration of this pull request has been delegated and may be completed by any project committer using the /integrate pull request command.

iwanowww

Looks good.

iwanowww · 2024-10-01T23:51:15Z

/integrate

openjdk · 2024-10-01T23:52:49Z

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

83dcb02: 8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes
d2e7708: 8341367: Problemlist ShapeNotSetSometimes.java on macOS
0314973: 8341060: Cleanup statics in HeapDumper
021bf63: 8340458: Open source additional Component tests (part 2)
9a7817b: 8340988: Update jdk/jfr/event/gc/collection tests to accept "CodeCache GC Threshold" as valid GC reason
f2a767f: 8340907: Open source closed frame tests # 2
7b1e6f8: 8337389: Parallel: Remove unnecessary forward declarations in psScavenge.hpp
2120a84: 8341333: [JVMCI] Export JavaThread::_unlocked_inflated_monitor to JVMCI
684d246: 8341242: Shenandoah: LRB node is not matched as GC barrier after JDK-8340183
7cc7c08: 8337493: [JVMCI] Number of libgraal threads might be too low
... and 94 more: https://git.openjdk.org/jdk/compare/279086d4ce7e05972e099022e8045f39680dd4e8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-01T23:53:05Z

@iwanowww Pushed as commit 8d6d37f.

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

JDK-8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_…

5df1873

…access

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jul 4, 2024

tobiasholenstein changed the title ~~JDK-8320308: C2 compilation crashes in LibraryCallKit:: inline_unsafe_acces~~ JDK-8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access Jul 4, 2024

openjdk bot changed the title ~~JDK-8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access~~ 8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access Jul 4, 2024

Tobias Holenstein added 10 commits July 5, 2024 11:11

jtreg test

1d275f7

fix

1f66b5d

add IgnoreUnrecognizedVMOptions

3d5d51e

Update UnsafeArrayAccess.java

812523b

remove UnlockDiagnosticVMOptions

more comments

2756ad7

improve comments

c06641d

comments

c17fa91

Update UnsafeArrayAccess.java

efed700

move test

ee67a39

Merge branch 'JDK-8320308' of github.com:tobiasholenstein/jdk into JD…

776204b

…K-8320308

Tobias Holenstein added 2 commits July 8, 2024 15:42

update CompileCommand

0e14b3e

less iterantions

34c6e0d

openjdk bot removed the graal graal-dev@openjdk.org label Jul 9, 2024

tobiasholenstein marked this pull request as ready for review July 9, 2024 17:36

openjdk bot added the rfr Pull request is ready for review label Jul 9, 2024

Tobias Holenstein added 3 commits September 24, 2024 15:47

Merge remote-tracking branch 'origin/master' into JDK-8320308

eff6644

Merge remote-tracking branch 'origin/master' into JDK-8320308

efcd729

from JDK-8340707 from ProblemList

79d8e96

openjdk bot removed the ready Pull request is ready to be integrated label Sep 24, 2024

JTreg style

2abec7d

eme64 approved these changes Sep 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 24, 2024

TobiHartmann approved these changes Sep 25, 2024

View reviewed changes

TobiHartmann reviewed Sep 25, 2024

View reviewed changes

test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithNullBase.java Outdated Show resolved Hide resolved

Update test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithN…

255c0de

…ullBase.java Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Sep 25, 2024

vnkozlov reviewed Sep 25, 2024

View reviewed changes

remove othervm in TestUnsafeArrayAccessWithNullBase.java

85f0b32

add second uncast (Vladimirs suggestion)

4abc6f1

vnkozlov approved these changes Sep 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 26, 2024

openjdk bot added the delegated label Sep 27, 2024

TobiHartmann approved these changes Sep 30, 2024

View reviewed changes

iwanowww approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 1, 2024

openjdk bot closed this Oct 1, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review delegated labels Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access #20033

8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access #20033

tobiasholenstein commented Jul 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 4, 2024

openjdk bot commented Jul 4, 2024 •

edited

Loading

openjdk bot commented Jul 4, 2024

openjdk bot commented Jul 8, 2024

tobiasholenstein commented Jul 9, 2024

openjdk bot commented Jul 9, 2024

mlbridge bot commented Jul 9, 2024 •

edited

Loading

vnkozlov commented Jul 9, 2024

iwanowww commented Jul 9, 2024 •

edited

Loading

tobiasholenstein commented Jul 10, 2024

eme64 left a comment

TobiHartmann left a comment

vnkozlov Sep 25, 2024

tobiasholenstein Sep 26, 2024

tobiasholenstein commented Sep 26, 2024

vnkozlov left a comment

tobiasholenstein commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

iwanowww left a comment

iwanowww commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

	BasicType bt = alias_type->basic_type();
	if (bt != T_ILLEGAL) {

8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access #20033

8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access #20033

Conversation

tobiasholenstein commented Jul 4, 2024 • edited by openjdk bot Loading

Fix (updated on 18th Sep 2024)

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 4, 2024

openjdk bot commented Jul 4, 2024 • edited Loading

openjdk bot commented Jul 4, 2024

openjdk bot commented Jul 8, 2024

tobiasholenstein commented Jul 9, 2024

openjdk bot commented Jul 9, 2024

mlbridge bot commented Jul 9, 2024 • edited Loading

Webrevs

vnkozlov commented Jul 9, 2024

iwanowww commented Jul 9, 2024 • edited Loading

tobiasholenstein commented Jul 10, 2024

eme64 left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

vnkozlov Sep 25, 2024

Choose a reason for hiding this comment

tobiasholenstein Sep 26, 2024

Choose a reason for hiding this comment

tobiasholenstein commented Sep 26, 2024

vnkozlov left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

iwanowww left a comment

Choose a reason for hiding this comment

iwanowww commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

tobiasholenstein commented Jul 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 4, 2024 •

edited

Loading

mlbridge bot commented Jul 9, 2024 •

edited

Loading

iwanowww commented Jul 9, 2024 •

edited

Loading