-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8297763: Fix missing stub code expansion before align() in shared trampolines #11414
Conversation
👋 Welcome back xlinzheng! A progress list of the required criteria for merging this PR into |
@zhengxiaolinX The following label will be automatically applied to this pull request:
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. |
Webrevs
|
@@ -52,6 +52,11 @@ static bool emit_shared_trampolines(CodeBuffer* cb, CodeBuffer::SharedTrampoline | |||
bool p_succeeded = true; | |||
auto emit = [&](address dest, const CodeBuffer::Offsets &offsets) { | |||
masm.set_code_section(cb->stubs()); | |||
if (cb->stubs()->maybe_expand_to_ensure_remaining(NativeInstruction::instruction_size) && cb->blob() == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoud we add a check for the real code alignment here and put maybe_expand_to_ensure_remaining() and masm.align(wordSize) operations under that check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion - yes, indeed.
Gentle ping - may I have a second review please (the AArch64 part of the changes)? The latest change has passed hotspot tier1~tier4 on both platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
@zhengxiaolinX 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:
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 104 new commits pushed to the
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) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Thanks for the reviews @RealFYang @luhenry! (The GHA failure is not related) /integrate |
@zhengxiaolinX |
/sponsor |
Going to push as commit f9e0f1d.
Your commit was automatically rebased without conflicts. |
@TobiHartmann @zhengxiaolinX Pushed as commit f9e0f1d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Thank you, Tobias! |
This patch fixes missing stub code expansion logic before
align()
for AArch64 and RISC-V.The
align()
at most creates 4-byte padding, so aNativeInstruction::instruction_size
is enough.I am considering pre-calculating the total trampoline sizes and allocating them in batches, but maybe after this one, for this is a quick fix to unblock #11188. Please see that thread.
The
assert_alignment(pc());
added in the RISC-V part shows that RVC doesn't change the trampoline stub / static stub logic, so there is no need to adjust the trampoline size for it. [1]Tested AArch64 hotspot tier1~3, and 4 is still running; tested RISC-V hotspot tier1~2, and 3~4 are still running.
Thanks,
Xiaolin
[1]
jdk/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
Lines 3125 to 3126 in 2deb318
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11414/head:pull/11414
$ git checkout pull/11414
Update a local copy of the PR:
$ git checkout pull/11414
$ git pull https://git.openjdk.org/jdk pull/11414/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11414
View PR using the GUI difftool:
$ git pr show -t 11414
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11414.diff