8303153: Native interpreter frame missing mirror

[fbredber]

The mirror needs to be stored in the frame for native calls also on AArch64 and RISC-V (as it is on other platforms).
See JDK-8303153 for more info.
Passes tier1-5 tests on AArch64. Done basic tests on RISC-V using QEmu.

---

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-8303153: Native interpreter frame missing mirror

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 3d0a5ff9
• Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13794

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

• 01: Full - Incremental (6a8c18a7)
• 00: Full (3d0a5ff9)

[fbredber]

I'd appreciate if @theRealAph and @RealFYang can have a look at this.

[coleenp]

This looks good to me.

[openjdk]

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

8303153: Native interpreter frame missing mirror

Reviewed-by: coleenp, fyang

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

• 5c7ede9: 8307347: serviceability/sa/ClhsdbDumpclass.java could leave files owned by root on macOS
• 89b7d07: 8307100: Remove ReferentBasedDiscovery reference discovery policy
• f6ea897: 8306285: Missing file in search test
• 33245d6: 8307517: Add VMErrorCallback infrastructure to extend hs_err dumping
• 7a1cb64: 8293547: Add relaxed add_and_fetch for macos aarch64 atomics
• 68f385c: 8307428: jstat tests doesn't tolerate dash in the O column
• 8bbd264: 8307423: [s390x] Represent Registers as values
• 959e62c: 8307521: Introduce check_oop infrastructure to check oops in the oop class
• ad0e5a9: 8304720: SuperWord::schedule should rebuild C2-graph from SuperWord dependency-graph
• 495f268: 8306966: RISC-V: Support vector cast node for Vector API
• ... and 158 more: https://git.openjdk.org/jdk/compare/35e7bc21d3c1b38e2268924b20ae4b149b4f8cd8...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp, @RealFYang) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[RealFYang]

Hi,
Thanks for taking care of RISC-V at the same time.
I think it would be cleaner if we do following for the RISC-V part:

diff --git a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
index 3e0e94515fc..f8ce528634f 100644
--- a/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
+++ b/src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
@@ -770,8 +770,10 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
   __ sd(x19_sender_sp, Address(sp, 9 * wordSize));
   __ sd(zr, Address(sp, 8 * wordSize));

-  // Get mirror
+  // Get mirror and store it in the frame as GC root for this Method*
   __ load_mirror(t2, xmethod, x15, t1);
+  __ sd(t2, Address(sp, 4 * wordSize));
+
   if (!native_call) {
     __ ld(t0, Address(xmethod, Method::const_offset()));
     __ lhu(t0, Address(t0, ConstMethod::max_stack_offset()));
@@ -779,9 +781,8 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
     __ slli(t0, t0, 3);
     __ sub(t0, sp, t0);
     __ andi(t0, t0, -16);
-    // Store extended SP and mirror
+    // Store extended SP
     __ sd(t0, Address(sp, 5 * wordSize));
-    __ sd(t2, Address(sp, 4 * wordSize));
     // Move SP out of the way
     __ mv(sp, t0);
   } else {
@@ -789,7 +790,6 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
     // 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);
   }
 }

[RealFYang]

Looks reasonable to me. Thanks for the update.
FYI: I have performed tier1-3 tests on linux-riscv64 board, result looks good.

[fbredber]

Thanks for the review @coleenp and @RealFYang.
If no one else has anything to add, I'll integrate (as soon as I can get a sponsor).

[fbredber]

/integrate

[openjdk]

@fbredber
Your change (at version 6a8c18a) is now ready to be sponsored by a Committer.

[coleenp]

/sponsor

[openjdk]

Going to push as commit 5a259d8.
Since your change was applied there have been 175 commits pushed to the master branch:

• 4b02956: 8054022: HttpURLConnection timeouts with Expect: 100-Continue and no chunking
• 9f34e4f: 8304434: [AIX] Update minimum xlclang version
• d2e0e53: 8307604: gcc12 based Alpine build broken build after JDK-8307301
• e91f0d3: 8307571: Remove unused SomeConstants in WatcherThread class
• 26755a9: 8306408: Fix the format of several tables in building.md
• 64c0962: 8307569: Build with gcc8 is broken after JDK-8307301
• bb3e44d: 8307603: [AIX] Broken build after JDK-8307301
• 5c7ede9: 8307347: serviceability/sa/ClhsdbDumpclass.java could leave files owned by root on macOS
• 89b7d07: 8307100: Remove ReferentBasedDiscovery reference discovery policy
• f6ea897: 8306285: Missing file in search test
• ... and 165 more: https://git.openjdk.org/jdk/compare/35e7bc21d3c1b38e2268924b20ae4b149b4f8cd8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@coleenp @fbredber Pushed as commit 5a259d8.

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