8303279: C2 Compiler crash (triggered by Kotlin 1.8.10)

[simonis]

This is a problem probably introduced by JDK-8238691. It could reproduce it with JDK 17, 18 and 21 and results in the following crash (see JBS-issue for more details):

# Internal Error (/priv/simonisv/OpenJDK/Git/jdk/src/hotspot/share/opto/type.hpp:2059), pid=1152816, tid=1154124
# assert(_base >= OopPtr && _base <= AryPtr) failed: Not a Java pointer
#
# JRE version: OpenJDK Runtime Environment (21.0) (slowdebug build 21-internal-adhoc.simonisv.jdk)
...
Current CompileTask:
C2: 91009 8214 ! 4 io.grpc.kotlin.ServerCalls$serverCallListener$requests$1::invokeSuspend (285 bytes)

Stack: [0x00007fff1306b000,0x00007fff1316c000], sp=0x00007fff13166fe0, free space=1007k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x61d636] Type::is_oopptr() const+0x4e (type.hpp:2059)
V [libjvm.so+0x14cee55] SubTypeCheckNode::sub(Type const*, Type const*) const+0x53 (subtypenode.cpp:37)
V [libjvm.so+0x14c66b0] SubNode::Value(PhaseGVN*) const+0xa6 (subnode.cpp:107)
V [libjvm.so+0xcdacb3] split_if(IfNode*, PhaseIterGVN*)+0x2ce (ifnode.cpp:111)
V [libjvm.so+0xce044c] IfNode::Ideal_common(PhaseGVN*, bool)+0x128 (ifnode.cpp:1438)
V [libjvm.so+0xce0496] IfNode::Ideal(PhaseGVN*, bool)+0x30 (ifnode.cpp:1448)
V [libjvm.so+0x1298244] PhaseGVN::apply_ideal(Node*, bool)+0x70 (phaseX.cpp:667)
V [libjvm.so+0x129a0fd] PhaseIterGVN::transform_old(Node*)+0x12d (phaseX.cpp:1196)
V [libjvm.so+0x12998df] PhaseIterGVN::optimize()+0x16b (phaseX.cpp:1045)
V [libjvm.so+0x93f89e] Compile::Optimize()+0xce0 (compile.cpp:2378)
V [libjvm.so+0x9385fa] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x16ca (compile.cpp:842)
V [libjvm.so+0x806ab4] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x1a0 (c2compiler.cpp:118)
V [libjvm.so+0x958bc8] CompileBroker::invoke_compiler_on_method(CompileTask*)+0xa04 (compileBroker.cpp:2265)
V [libjvm.so+0x9576fa] CompileBroker::compiler_thread_loop()+0x462 (compileBroker.cpp:1944)
V [libjvm.so+0x97b14a] CompilerThread::thread_entry(JavaThread*, JavaThread*)+0x84 (compilerThread.cpp:58)
V [libjvm.so+0xd434ce] JavaThread::thread_main_inner()+0x15c (javaThread.cpp:719)
V [libjvm.so+0xd43368] JavaThread::run()+0x258 (javaThread.cpp:704)
V [libjvm.so+0x15481ea] Thread::call_run()+0x1a8 (thread.cpp:217)
V [libjvm.so+0x1230036] thread_native_entry(Thread*)+0x1a5 (os_linux.cpp:778)
...

SubTypeCheckNode::sub() expects that it's sub_t input Type is either a Klasspointer (i.e. Type::KlassPtr) or an Ooppointer (i.e. Type::OopPtr, Type::InstPtr or Type::AryPtr). It only checks for a Klasspointer and if that's not the case it assumes an Ooppointer. However, in the crashing case, sub_t has the generic pointer type Type::AnyPtr so debug builds will run into an assertion and product builds will just crash.

The SubTypeCheckNode in question has the following shape in split_if():

 Con (#top)
  |
  | __IfTrue
  |/
  || __IfFalse
  |//
Region
  | __ ConP (#NULL)
  | /
  | __/ _ Phi (Oop:kotlinx/coroutines/internal/LockFreeLinkedListNode:NotNull)
  || ___/
  ||| ____ Phi (Oop:kotlinx/coroutines/internal/LockFreeLinkedListNode:NotNull)
  ||||
  |///
  Phi
  | ConP (Klass:precise klass kotlinx/coroutines/channels/Send)
  | |
   \ /
  SubTypeCheck

split_if() then searches for the first contstant input pf SubTypeCheck Phi-node and finds ConP (#NULL). It then calls SubTypeCheckNode::sub() with sub_t as ConP (#NULL)'s type which is Type::AnyPtr and crashes.

I've verified that returning bottom_type() from SubTypeCheckNode::sub for the (!sub_t->isa_klassptr() && !sub_t->isa_oopptr()) case fixes the crash (by instrumenting the VM to ensure that the compilation as well as the further program execution succeeds if we take the new branch).

I'm only not sure if the unusual graph which leads to this crash is caused by the uncommon bytecode generated by the Kotlin compiler or if it is the result of another problem in an earlier optimization stage?

While browsing JBS, I found JDK-8303513 which seems similar to this issue (i.e. also caused by a SubTypeCheckNode with an input of the TOP constant node).

While looking at SubTypeCheckNode::Ideal() I found that it already has exactly the same safeguard as proposed for SubTypeCheckNode::sub() in this PR, namely:

  if (!super_t->isa_klassptr() ||
      (!sub_t->isa_klassptr() && !sub_t->isa_oopptr())) {
    return NULL;
  }

I'd really appreciate if @rwestrel could take a look at this issue.

---

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-8303279: C2 Compiler crash (triggered by Kotlin 1.8.10) (Bug - P3)

Contributors

• Roland Westrelin <roland@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14600

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@simonis The following label will be automatically applied to this pull request:

• hotspot-compiler

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 (7a73d39c)
• 00: Full (9e8d10a4)

[rwestrel]

@simonis I reproduced it and I'm taking a closer look.

[simonis]

I've added @rwestrel's test to the PR and verified that the code generated for the new test with this fix is the same like the code that was generated before JDK-8238691 and also the same like the code generated if we run with -XX:+StressIGVN but a different StressSeed.

[simonis]

/contributor add roland

[openjdk]

@simonis
Contributor Roland Westrelin <roland@openjdk.org> successfully added.

[rwestrel]

FWIW, I propose an alternate fix here. master...rwestrel:jdk:JDK-8303279
Seeing null or a nullable value at a SubTypeCheck could be a bug as the expectation is that inputs are null checked and the implementation of SubTypeCheck would crash with a null input. So I added an assert to SubTypeCheckNode::sub to catch a nullable input. The assert fires with the test because split if runs with a non yet fully collapsed dead path. So I tweak split if so it's delayed until the path is collapsed. When running testing I found that the assert would fire in other cases because of values known to be non null be not marked as such. The end result is a bigger patch.

[simonis]

FWIW, I propose an alternate fix here. master...rwestrel:jdk:JDK-8303279 Seeing null or a nullable value at a SubTypeCheck could be a bug as the expectation is that inputs are null checked and the implementation of SubTypeCheck would crash with a null input. So I added an assert to SubTypeCheckNode::sub to catch a nullable input. The assert fires with the test because split if runs with a non yet fully collapsed dead path. So I tweak split if so it's delayed until the path is collapsed. When running testing I found that the assert would fire in other cases because of values known to be non null be not marked as such. The end result is a bigger patch.

Thanks @rwestrel. I'm fine with your patch. Do you want to take JDK-8303279 and propose your fix as PR? I will then close mine.

[rwestrel]

Thanks @rwestrel. I'm fine with your patch. Do you want to take JDK-8303279 and propose your fix as PR? I will then close mine.

Let me open the PR. I will away for a week though starting later today.

[simonis]

Closing this PR in favour of @rwestrel 's.