8308583: SIGSEGV in GraphKit::gen_checkcast #14123

rwestrel · 2023-05-24T12:31:37Z

At an instanceof, a node of type bottom[int:>=0] is checked to be of type cc$Word and on the success a CheckCastPP is inserted to change the node type. That CheckCastPP constant folds to top but the type check doesn't fold. The reason is that the type check loads the klass from the node with a LoadNKlass and the type of that node is java/lang/Object when it should be bottom[int:>=0] but logic in LoadNKlass::Value() gets in the way.

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-8308583: SIGSEGV in GraphKit::gen_checkcast

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to a886d516
Vladimir Kozlov (@vnkozlov - Reviewer)
Tom Rodriguez (@tkrodriguez - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14123

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-24T12:33:07Z

👋 Welcome back roland! 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 · 2023-05-24T12:35:38Z

@rwestrel 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 · 2023-05-25T06:49:11Z

Webrevs

01: Full - Incremental (75de9a72)
00: Full (a886d516)

TobiHartmann

Looks good to me.

openjdk · 2023-05-25T09:33:46Z

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

8308583: SIGSEGV in GraphKit::gen_checkcast

Reviewed-by: thartmann, kvn, never

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

89b3c37: 8307125: compiler/jvmci/compilerToVM/MaterializeVirtualObjectTest.java hits assert(!Continuation::is_frame_in_continuation(thread(), fr())) failed: No support for deferred values in continuations
98acce1: 8306703: JFR: Summary views
534de6d: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
48d21bd: 8286597: Implement PollerProvider on AIX
e7edf8d: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
90e57fd: 8308335: JFR: Remove @experimental from Virtual Threads events
7e2e05d: 8308098: G1: Remove redundant checks in G1ObjectCountIsAliveClosure
2599ada: 8308655: Narrow types of ConstantPool and ConstMethod returns
5a0a238: 8308746: C2 IR test failures for TestFpMinMaxReductions.java with SSE2
38367d3: 8308735: Typos in parameter names
... and 74 more: https://git.openjdk.org/jdk/compare/05e99db466e7ef5c26f089db772a21cb2ca62e93...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.

tkrodriguez · 2023-05-25T15:15:33Z

Thanks for the fix. I did wonder why the type checks themselves weren't properly folding but didn't follow it through all the way.

I think it would be worth including an assert or guarantee in type_check_receiver that it never injects top into the graph.

vnkozlov

In addition to normal mach5 testing we need to run our stress testing too for this changes to run with StressReflectiveCode on.

vnkozlov · 2023-05-25T15:48:39Z

src/hotspot/share/opto/graphKit.cpp

  const TypeKlassPtr* inst_klass = _gvn.type(klass_node)->isa_klassptr();
  if (!StressReflectiveCode && inst_klass != nullptr) {
    bool    xklass = inst_klass->klass_is_exact();
-    if (xklass || inst_klass->isa_aryklassptr()) {
+    if (xklass || (inst_klass->isa_aryklassptr() && inst_klass->is_aryklassptr()->elem() != Type::BOTTOM)) {


Can you rename inst_klass to klass_ptr because it is general klass. It is weird to see a check like inst_klass->isa_aryklassptr().

I pushed a new commit that includes the rename.

rwestrel · 2023-05-25T16:00:52Z

I think it would be worth including an assert or guarantee in type_check_receiver that it never injects top into the graph.

Right. I included one in the new commit I just pushed.

vnkozlov

Good.

rwestrel · 2023-05-26T07:02:38Z

@TobiHartmann @vnkozlov @tkrodriguez thanks for the reviews

rwestrel · 2023-05-26T07:02:46Z

/integrate

openjdk · 2023-05-26T07:03:44Z

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

46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
7d2a7ce: 8308672: Add version number in the replay file generated by DumpInline
27ba8bd: 8308108: Support Unicode extension for collation settings
89b3c37: 8307125: compiler/jvmci/compilerToVM/MaterializeVirtualObjectTest.java hits assert(!Continuation::is_frame_in_continuation(thread(), fr())) failed: No support for deferred values in continuations
98acce1: 8306703: JFR: Summary views
534de6d: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
48d21bd: 8286597: Implement PollerProvider on AIX
... and 80 more: https://git.openjdk.org/jdk/compare/05e99db466e7ef5c26f089db772a21cb2ca62e93...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-26T07:04:05Z

@rwestrel Pushed as commit 199b1bf.

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

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 24, 2023

fix

839d73f

rwestrel force-pushed the JDK-8308583 branch from 6522dea to 839d73f Compare May 24, 2023 13:25

test

a886d51

rwestrel marked this pull request as ready for review May 25, 2023 06:43

openjdk bot added the rfr Pull request is ready for review label May 25, 2023

TobiHartmann approved these changes May 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 25, 2023

vnkozlov reviewed May 25, 2023

View reviewed changes

review

75de9a7

vnkozlov approved these changes May 25, 2023

View reviewed changes

tkrodriguez approved these changes May 25, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 26, 2023

openjdk bot closed this May 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8308583: SIGSEGV in GraphKit::gen_checkcast #14123

8308583: SIGSEGV in GraphKit::gen_checkcast #14123

rwestrel commented May 24, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 24, 2023

openjdk bot commented May 24, 2023

mlbridge bot commented May 25, 2023 •

edited

Loading

TobiHartmann left a comment

openjdk bot commented May 25, 2023 •

edited

Loading

tkrodriguez commented May 25, 2023

vnkozlov left a comment

vnkozlov May 25, 2023 •

edited

Loading

rwestrel May 25, 2023

rwestrel commented May 25, 2023

vnkozlov left a comment

rwestrel commented May 26, 2023

rwestrel commented May 26, 2023

openjdk bot commented May 26, 2023

openjdk bot commented May 26, 2023

8308583: SIGSEGV in GraphKit::gen_checkcast #14123

8308583: SIGSEGV in GraphKit::gen_checkcast #14123

Conversation

rwestrel commented May 24, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 24, 2023

openjdk bot commented May 24, 2023

mlbridge bot commented May 25, 2023 • edited Loading

Webrevs

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk bot commented May 25, 2023 • edited Loading

tkrodriguez commented May 25, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov May 25, 2023 • edited Loading

Choose a reason for hiding this comment

rwestrel May 25, 2023

Choose a reason for hiding this comment

rwestrel commented May 25, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

rwestrel commented May 26, 2023

rwestrel commented May 26, 2023

openjdk bot commented May 26, 2023

openjdk bot commented May 26, 2023

rwestrel commented May 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 25, 2023 •

edited

Loading

openjdk bot commented May 25, 2023 •

edited

Loading

vnkozlov May 25, 2023 •

edited

Loading