Skip to content
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

8294359: Interpreter(AArch64) intrinsify Thread.currentThread() #10441

Closed

Conversation

shqking
Copy link
Contributor

@shqking shqking commented Sep 27, 2022

JDK-8278793 supported Thread.currentThread() intrinsification for interpreter(x64) part so as to get JVM startup benefit.

In this patch, we implement the AArch64 part.

Testings: tier1~3 and jdk_loom passed on Linux/AArch64.


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-8294359: Interpreter(AArch64) intrinsify Thread.currentThread()

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10441

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

Using diff file

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

JDK-8278793 supported Thread.currentThread() intrinsification for
interpreter(x64) part so as to get JVM startup benefit.

In this patch, we implement the AArch64 part.

Testings: tier1~3 and jdk_loom passed on Linux/AArch64.
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 27, 2022

👋 Welcome back haosun! 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 openjdk bot added the rfr Pull request is ready for review label Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@shqking 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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 27, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 27, 2022

Webrevs

Copy link
Contributor

@theRealAph theRealAph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk
Copy link

openjdk bot commented Sep 27, 2022

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

8294359: Interpreter(AArch64) intrinsify Thread.currentThread()

Reviewed-by: aph, shade

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

  • 94e14da: 8294057: Parallel: Tighten ParallelCompactData::initialize_region_data
  • 1ea0d6b: 8292301: [REDO v2] C2 crash when allocating array of size too large
  • c13e0ef: 8292848: AWT_Mixing and TrayIcon tests fail on el8 with hard-coded isOel7
  • 79ccc79: 8293613: need to properly handle and hide tmp VTMS transitions
  • 5e1e449: 8290920: sspi_bridge.dll not built if BUILD_CRYPTO is false
  • d827fd8: 8294430: RISC-V: Small refactoring for movptr_with_offset
  • 9d76ac8: 8292158: AES-CTR cipher state corruption with AVX-512
  • e5b65c4: 8290482: Update JNI Specification of DestroyJavaVM for better alignment with JLS, JVMS, and Java SE API Specifications
  • f8d9fa8: 8294483: Remove vmTestbase/nsk/jvmti/GetThreadState tests.
  • 6ad151d: 8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference
  • ... and 11 more: https://git.openjdk.org/jdk/compare/dd51f7e0b75d3a16403608d89cd206ac0bedf882...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 (@theRealAph, @shipilev) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 27, 2022
Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. (I had to look up definitions for rscratch1 and rscratch2 to make sure they do not collide with r0)

@shqking
Copy link
Contributor Author

shqking commented Sep 28, 2022

Thanks for your review!

I think the GHA failure is due to the network issue, not related to this PR.
/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 28, 2022
@openjdk
Copy link

openjdk bot commented Sep 28, 2022

@shqking
Your change (at version a10f47d) is now ready to be sponsored by a Committer.

@nsjian
Copy link

nsjian commented Sep 28, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 28, 2022

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

  • c42ef70: 7148092: [macosx] When Alt+down arrow key is pressed, the combobox popup does not appear.
  • 94e14da: 8294057: Parallel: Tighten ParallelCompactData::initialize_region_data
  • 1ea0d6b: 8292301: [REDO v2] C2 crash when allocating array of size too large
  • c13e0ef: 8292848: AWT_Mixing and TrayIcon tests fail on el8 with hard-coded isOel7
  • 79ccc79: 8293613: need to properly handle and hide tmp VTMS transitions
  • 5e1e449: 8290920: sspi_bridge.dll not built if BUILD_CRYPTO is false
  • d827fd8: 8294430: RISC-V: Small refactoring for movptr_with_offset
  • 9d76ac8: 8292158: AES-CTR cipher state corruption with AVX-512
  • e5b65c4: 8290482: Update JNI Specification of DestroyJavaVM for better alignment with JLS, JVMS, and Java SE API Specifications
  • f8d9fa8: 8294483: Remove vmTestbase/nsk/jvmti/GetThreadState tests.
  • ... and 12 more: https://git.openjdk.org/jdk/compare/dd51f7e0b75d3a16403608d89cd206ac0bedf882...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 28, 2022
@openjdk openjdk bot closed this Sep 28, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 28, 2022
@openjdk
Copy link

openjdk bot commented Sep 28, 2022

@nsjian @shqking Pushed as commit ea61671.

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

@shqking shqking deleted the 8294359-aarch64-currentThread branch September 28, 2022 11:49
shqking added a commit to shqking/jdk that referenced this pull request Apr 4, 2023
…c-ret

* Background

1. PAC-RET branch protection was initially implemented on Linux/AArch64
in JDK-8277204 [1].

2. However, it was broken with the introduction of virtual threads [2],
mainly because the continuation freeze/thaw mechanism would trigger
stack copying to/from memory, whereas the saved and signed LR on the
stack doesn't get re-signed accordingly.

3. PR-9067 [3] tried to implement the re-sign part, but it was not
accepted because option "PreserveFramePointer" is always turned on by
PAC-RET but this would slow down virtual threads by ~5-20x.

4. As a workaround, JDK-8288023 [4] disables PAC-RET when preview
language features are enabled. Note that virtual thread is one preview
feature then.

5. Virtual thread will become a permanent feature in JDK-21 [5][6].

* Goal

This patch aims to make PAC-RET compatible with virtual threads.

* Requirements of virtual threads

R-1: Option "PreserveFramePointer" should be turned off. That is,
PAC-RET implementation should not rely on frame pointer FP. Otherwise,
the fast path in stack copying will never be taken.

R-2: Use some invariant values to stack copying as the modifier, so as
to avoid the PAC re-sign for continuation thaw, as the fast path in
stack copying doesn't walk the frame.

Note that more details can be found in the discussion [3].

* Investigation

We considered to use (relative) stack pointer SP, thread ID, PACStack
[7] and value zero as the candidate modifier.

1. SP: In some scenarios, we need to authenticate the return address in
places where the current SP doesn't match the SP on function entry. E.g.
see the usage in Runtime1::generate_handle_exception(). Hence, neither
absolute nor relative SP works.

2. thread ID (tid): It's invariant to virtual thread, but it's
nontrivial to access it from the JIT side. We need 1) firstly resolve
the address of current thread (See [8] as an example), and 2) get the
tid field in the way like java_lang_Thread::thread_id(). I suppose this
would introduce big performance overhead.

Then can we turn to use "rthread" register (JavaThread object address)
as the modifier? Unfortunately, it's not an invariant to virtual threads
and PAC re-sign is still needed.

3. PACStack uses the signed return address of caller as the modifier to
sign the callee's return address. In this way, we get one PACed call
chain. The modifier should be saved into somewhere around the frame
record. Inevitably, FP should be preserved to make it easy to find this
modifier in case of some exception scenarios (Recall the reason why we
fail to use SP as the modifier).

Finally, we choose to use value zero as the modifier. Trivially, it's
compatible with virtual threads. However, compared to FP modifier, this
solution would reduce the strength of PAC-RET protection to some extent.
E.g., you get the same authentication code for each call to the
function, whereas using FP gives you different codes as long as the
stack depth is different.

* Implementation of Zero modifier

Here list the key updates of this patch.

1. vm_version_aarch64.cpp

Remove the constraint on "enable-preview" and "PreserveFramePointer".

2. macroAssembler_aarch64.cpp

For utility protect_return_address(), 1) use PACIAZ/PACIZA instructions
directly. 2) argument "temp_reg" is removed since all functions use the
same modifier. 3) all the use sites are updated accordingly. This
involves the updates in many files.

Similar updates are done to utility authenticate_return_address().

Besides, aarch64.ad and AArch64TestAssembler.java are updated
accordingly.

3. pauth_linux_aarch64.inline.hpp

For utilities pauth_sign_return_address() and
pauth_authenticate_return_address(), remove the second argument and pass
value zero to r16 register.

Similarly, all the use sites are updated as well. This involves the
updates in many files.

4. continuationHelper_aarch64.inline.hpp

Introduce return_pc_at() and patch_pc_at() to avoid directly
reading the saved PC or writing new signed PC on the stack in shared
code.

5. Minor updates

1) sharedRuntime_aarch64.cpp: Add the missing
authenticate_return_address() use for function gen_continuation_enter().
In functions generate_deopt_blob() and generate_uncommon_trap_blob(),
remove the authentication on the caller (3) frame since the return
address is not used.

2) stubGenerator_aarch64.cpp: Add the missing
authenticate_return_address() use for function generate_cont_thaw().

3) runtime.cpp: enable the authentication.

* Test

1. Cross compilations on arm32/s390/ppc/riscv passed.
2. zero build and x86 build passed.
3. tier1~3 passed on Linux/AArch64 w/ and w/o PAC-RET.

Co-Developed-by: Nick Gasson <Nick.Gasson@arm.com>

[1] https://bugs.openjdk.org/browse/JDK-8277204
[2] https://openjdk.org/jeps/425
[3] openjdk#9067
[4] https://bugs.openjdk.org/browse/JDK-8288023
[5] https://bugs.openjdk.org/browse/JDK-8301819
[6] https://openjdk.org/jeps/444
[7] https://www.usenix.org/conference/usenixsecurity21/presentation/liljestrand
[8] openjdk#10441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants