8305247: On RISC-V generate_fixed_frame() sometimes generate a relativized locals value which is way too large

[fbredber]

The relativized locals value is supposed to contain the distance between the frame pointer and the local variables in an interpreter frame, expressed in number of words. It typically contains the value "frame::sender_sp_offset + padding + max_locals - 1"

On most architectures sender_sp_offset is 2. This gives us the value "1 + padding + max_locals", which is always greater or equal to 1.

However on RISC-V the value of frame::sender_sp_offset is 0, which means that if we don't have any padding and no local variables we end up with a relativized_locals value of -1.

When generate_fixed_frame() calculates the relativized_locals value it subtracts the frame pointer from the xlocals and then logically shifts the result right by Interpreter::logStackElementSize (to convert it into a word index).

This works fine on all platforms (except RISC-V), because the subtraction will never become negative. But since the subtraction can end up negative on RISC-V, the shift instruction must be a arithmetic-shift-right (not a logical-shift-right) to preserve the sign and not end up with a very large positive index.

This is currently not a real problem since the relativized_locals value is not used if max_local is zero, which is the only case the value is wrong.

It is however a real problem when implementing JDK-8300197.

The bug was introduced in JDK-8299795 and is fixed by changing a "srli" instruction to a "srai" in generate_fixed_frame().

---

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-8305247: On RISC-V generate_fixed_frame() sometimes generate a relativized locals value which is way too large

Reviewers

• Fei Yang (@RealFYang - Reviewer)
• Robbin Ehn (@robehn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13245

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

Using diff file

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

[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.

[fbredber]

Hi @RealFYang can you verify that this PR works on RISC-V?
I've done sanity checking using Qemu, but nothing extensive.

[mlbridge]

Webrevs

• 00: Full (51e94995)

[RealFYang]

Looks reasonable. I performed tier1-3 tests on my linux-riscv64 boards, result looks good.
BTW: You might want to change the issue title removing the '.' symbol.

[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:

8305247: On RISC-V generate_fixed_frame() sometimes generate a relativized locals value which is way too large

Reviewed-by: fyang, rehn

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

• 790aced: 8305100: [REDO] Clean up JavadocTokenizer
• 2e91585: 8303123: Add line break opportunity to single type parameters
• 094e03d: 8299718: JavaDoc: Buttons to copy specific documentation URL are not accessible
• 4de24cd: 8303210: [linux, Windows] Make UseSystemMemoryBarrier available as product flag
• 336a23e: 8303229: JFR: Preserve disk repository after exit
• ecec611: 8283404: [macos] a11y : Screen magnifier does not show JMenu name
• aa76210: 8304893: Link Time Optimization with gcc can be faster
• b8c748d: 8294266: Add a way to pre-touch java thread stacks
• 41a3db2: 8304815: Use NMT for more precise hs_err location printing
• 34e66ce: 8304295: harfbuzz build fails with GCC 7 after JDK-8301998
• ... and 24 more: https://git.openjdk.org/jdk/compare/9df20600592427550998c6685f103737e3115a51...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 (@RealFYang, @robehn) 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).

[robehn]

Thanks

[fbredber]

Looks reasonable. I performed tier1-3 tests on my linux-riscv64 boards, result looks good. BTW: You might want to change the issue title removing the '.' symbol.

Thanks for testing @RealFYang. I've changed the title as you suggested.

[fbredber]

/integrate

[openjdk]

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

[robehn]

/sponsor

[openjdk]

Going to push as commit 33d09e5.
Since your change was applied there have been 34 commits pushed to the master branch:

• 790aced: 8305100: [REDO] Clean up JavadocTokenizer
• 2e91585: 8303123: Add line break opportunity to single type parameters
• 094e03d: 8299718: JavaDoc: Buttons to copy specific documentation URL are not accessible
• 4de24cd: 8303210: [linux, Windows] Make UseSystemMemoryBarrier available as product flag
• 336a23e: 8303229: JFR: Preserve disk repository after exit
• ecec611: 8283404: [macos] a11y : Screen magnifier does not show JMenu name
• aa76210: 8304893: Link Time Optimization with gcc can be faster
• b8c748d: 8294266: Add a way to pre-touch java thread stacks
• 41a3db2: 8304815: Use NMT for more precise hs_err location printing
• 34e66ce: 8304295: harfbuzz build fails with GCC 7 after JDK-8301998
• ... and 24 more: https://git.openjdk.org/jdk/compare/9df20600592427550998c6685f103737e3115a51...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robehn @fbredber Pushed as commit 33d09e5.

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