8294744: AArch64: applications/kitchensink/Kitchensink.java crashed: assert(oopDesc::is_oop(obj)) failed: not an oop

[pchilano]

Please review the following patch. The value we set initially for extended_sp on natives frames doesn't account for the oop that could be pushed to the stack in case the method throws an exception. This can create a situation in Interpreter::_throw_exception_entry where we push an exception oop to the Java expression stack below the actual physical stack pointer. When JFR is present though a JavaThread could receive a suspend signal right after that push. On Linux aarch64, because there is no red zone defined (nor implemented it seems), the pushed oop gets overwritten during the setup and execution of the signal handler. This later leads to a crash when popping the oop back and rethrowing in the caller of the native method. There are more details in the bug comments.

To fix it I used the same technique we use for normal Java frames, i.e. add extra space to extended_sp when creating the frame to account for the max space needed.



I tested the patch by running Kitchensink.java around 150 times on mach5 with no failures (without the patch 50 runs would already show ~10 failures on average). I also run tiers1-6 for sanity check.

Thanks,
Patricio

---

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-8294744: AArch64: applications/kitchensink/Kitchensink.java crashed: assert(oopDesc::is_oop(obj)) failed: not an oop

Reviewers

• Andrew Haley (@theRealAph - Reviewer) ⚠️ Review applies to 2aeadb14
• Fei Yang (@RealFYang - Reviewer)
• Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Contributors

• Fei Yang <fyang@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 85

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk20/pull/85.diff

[bridgekeeper]

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

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

• hotspot

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

• 02: Full - Incremental (2e2f1070)
• 01: Full - Incremental (2aeadb14)
• 00: Full (bd0b3e84)

[openjdk]

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

8294744: AArch64: applications/kitchensink/Kitchensink.java crashed: assert(oopDesc::is_oop(obj)) failed: not an oop

Co-authored-by: Fei Yang <fyang@openjdk.org>
Reviewed-by: aph, fyang, dcubed

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

• 21d468e: 8299733: AArch64: "unexpected literal addressing mode" assertion failure with -XX:+PrintC1Statistics
• 5826a07: 8299693: Change to Xcode12.4+1.1 devkit for building on macOS at Oracle
• d49851a: 8299689: Make use of JLine for Console as "opt-in"
• 1f141bd: 8299705: JCov coverage runs depend on jdk_symbols

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.

[RealFYang]

Looks good to me.
JVM on linux-riscv64 bears the same problem as it has the same code flow as aarch64 on this part [1][2]. And linux-riscv64 doesn't have a redzone defined/implemented either, which I have verified with a small C program with inline assembly like you do for x86 and aarch64. So I have prepared a fix for linux-riscv64. This has passed sanity tests on HiFive Unmatched.
Could you please add this change while you are on it?

diff --git a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
index 49a2b87c232..213e360c20e 100644
--- a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
+++ b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
@@ -776,8 +776,12 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
     // Move SP out of the way
     __ mv(sp, t0);
   } else {
-    __ sd(sp, Address(sp, 5 * wordSize));
+    // Make sure there is room for the exception oop pushed in case method throws
+    // an exception (see TemplateInterpreterGenerator::generate_throw_exception())
+    __ sub(t0, sp, 2 * wordSize);
+    __ sd(t0, Address(sp, 5 * wordSize));
     __ sd(zr, Address(sp, 4 * wordSize));
+    __ mv(sp, t0);
   }
 }

[1] https://bugs.openjdk.org/browse/JDK-8290280
[2] openjdk/jdk@4dd236b

[pchilano]

Looks good to me. JVM on linux-riscv64 bears the same problem as it has the same code flow as aarch64 on this part [1][2]. And linux-riscv64 doesn't have a redzone defined/implemented either, which I have verified with a small C program with inline assembly like you do for x86 and aarch64. So I have prepared a fix for linux-riscv64. This has passed sanity tests on HiFive Unmatched. Could you please add this change while you are on it?

diff --git a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
index 49a2b87c232..213e360c20e 100644
--- a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
+++ b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
@@ -776,8 +776,12 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
     // Move SP out of the way
     __ mv(sp, t0);
   } else {
-    __ sd(sp, Address(sp, 5 * wordSize));
+    // Make sure there is room for the exception oop pushed in case method throws
+    // an exception (see TemplateInterpreterGenerator::generate_throw_exception())
+    __ sub(t0, sp, 2 * wordSize);
+    __ sd(t0, Address(sp, 5 * wordSize));
     __ sd(zr, Address(sp, 4 * wordSize));
+    __ mv(sp, t0);
   }
 }

[1] https://bugs.openjdk.org/browse/JDK-8290280 [2] openjdk/jdk@4dd236b

Sure, added.

[pchilano]

/contributor add @RealFYang

[openjdk]

@pchilano
Contributor Fei Yang <fyang@openjdk.org> successfully added.

[dcubed-ojdk]

Thumbs up. Wonderful analysis in the bug report. You may want to attach
your standalone reproducers to the bug report for future spelunkers.

Thanks for including the testing information.

[dcubed-ojdk]

I was going to request that you add a '.' to the end of this sentence,
but it seems that lack of punctuation is the prevalent style in this file.

[pchilano]

Thumbs up. Wonderful analysis in the bug report. You may want to attach your standalone reproducers to the bug report for future spelunkers.

Thanks for including the testing information.

Thanks Dan!

[pchilano]

Thanks for the reviews @theRealAph, @RealFYang and @dcubed-ojdk!

[pchilano]

/integrate

[openjdk]

Going to push as commit 151450e.
Since your change was applied there have been 4 commits pushed to the master branch:

• 21d468e: 8299733: AArch64: "unexpected literal addressing mode" assertion failure with -XX:+PrintC1Statistics
• 5826a07: 8299693: Change to Xcode12.4+1.1 devkit for building on macOS at Oracle
• d49851a: 8299689: Make use of JLine for Console as "opt-in"
• 1f141bd: 8299705: JCov coverage runs depend on jdk_symbols

Your commit was automatically rebased without conflicts.

[openjdk]

@pchilano Pushed as commit 151450e.

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