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

8288971: AArch64: Clean up stack and register handling in interpreter #9239

Closed
wants to merge 7 commits into from

Conversation

theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Jun 22, 2022

There are several places in the interpreter that could be improved.

  1. We use r13 to pass the caller's SP to a callee through adapters. r13 is not a callee-saved register in the native ABI, so this causes some complications. Use a callee-saved register.
  2. We frequently recalculate the location where the native SP needs to go. We have a spare slot in the interpreter frame, so we should calculate it once, when the frame is created, and use it.
  3. Related to 1, we should clearly label all the places where the caller's SP is passed to a callee.

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-8288971: AArch64: Clean up stack and register handling in interpreter

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9239

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

Using diff file

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

@theRealAph theRealAph marked this pull request as draft June 22, 2022 13:00
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 22, 2022

👋 Welcome back aph! 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
Copy link

openjdk bot commented Jun 22, 2022

@theRealAph 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 Jun 22, 2022
@theRealAph theRealAph marked this pull request as ready for review June 27, 2022 14:44
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 27, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 27, 2022

Webrevs

src/hotspot/cpu/aarch64/assembler_aarch64.hpp Show resolved Hide resolved

void check_extended_sp(const char* msg = "check extended SP") {
#ifdef ASSERT
Label L;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be two space indent here

Copy link
Contributor

@adinn adinn left a comment

Choose a reason for hiding this comment

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

A little bit of rework needed.

popframe_extra_args;
extended_sp = align_down(extended_sp, StackAlignmentInBytes);
interpreter_frame->interpreter_frame_set_extended_sp(extended_sp);

// All frames but the initial (oldest) interpreter frame we fill in have
// a value for sender_sp that allows walking the stack but isn't
// truly correct. Correct the value here.
Copy link
Contributor

Choose a reason for hiding this comment

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

The condition this comment leads into tests for

interpreter_frame->sender_sp() == interpreter_frame->interpreter_frame_sender_sp())

I'm not really clear how/where that actually happens? Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
First thing to realize is that this method is used only by deopt. The logic here removes the caller's args that it's pushed onto the stack, iff the caller is an interpreted method.
I think this is a hangover from x86. It doesn't hurt, but neither does it do anything useful AFAICS because we're going to restore the caller's SP from interpreter_frame_extended_sp and the caller's ESP from interpreter_frame_last_sp as soon as we return.
I guess it's possible that something somewhere that walks the stack depends on this logic, but if it does I can't find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I should have looked at the calling code. Ta!

src/hotspot/cpu/aarch64/frame_aarch64.cpp Show resolved Hide resolved
Copy link
Contributor

@adinn adinn left a comment

Choose a reason for hiding this comment

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

All good.

@openjdk
Copy link

openjdk bot commented Jun 30, 2022

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

8288971: AArch64: Clean up stack and register handling in interpreter

Reviewed-by: adinn, ngasson

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

  • feb223a: 8288707: javax/swing/JToolBar/4529206/bug4529206.java: setFloating does not work correctly
  • c3addbb: 8288895: LdapContext doesn't honor set referrals limit
  • 1305fb5: 8287984: AArch64: [vector] Make all bits set vector sharable for match rules
  • 7b5bd25: 8286397: Address possibly lossy conversions in jdk.hotspot.agent
  • 28c5e48: 8287094: IGV: show node input numbers in edge tooltips
  • da6d1fc: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
  • 31e50f2: 8286104: use aggressive liveness for unstable_if traps
  • dddd4e7: 8289291: HttpServer sets incorrect value for "max" parameter in Keep-Alive header value
  • 048bffa: Merge
  • cf71544: 8289252: Recommend Locale.of() method instead of the constructor
  • ... and 154 more: https://git.openjdk.org/jdk/compare/cc445926cfe41ee7803791fb223504b0d9fd8100...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 30, 2022
@theRealAph
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 4, 2022

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

  • d53b02e: 8287312: G1: Early return on first failure in VerifyRegionClosure
  • a8edd7a: 8289569: [test] java/lang/ProcessBuilder/Basic.java fails on Alpine/musl
  • e31003a: 8289575: G1: Remove unnecessary is-marking-active check in G1BarrierSetRuntime::write_ref_field_pre_entry
  • 8e7a3cb: 8289431: (zipfs) Avoid redundant HashMap.get in ZipFileSystemProvider.removeFileSystem
  • 649f2d8: 8065097: [macosx] javax/swing/Popup/TaskbarPositionTest.java fails because Popup is one pixel off
  • d8444aa: 8286610: Add additional diagnostic output to java/net/DatagramSocket/InterruptibleDatagramSocket.java
  • 70f5693: Merge
  • f5cdaba: 8245268: -Xcomp is missing from java launcher documentation
  • 9515560: 8288703: GetThreadState returns 0 for virtual thread that has terminated
  • cfc9a88: 8288854: getLocalGraphicsEnvironment() on for multi-screen setups throws exception NPE
  • ... and 184 more: https://git.openjdk.org/jdk/compare/cc445926cfe41ee7803791fb223504b0d9fd8100...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jul 4, 2022

@theRealAph Pushed as commit b5d9656.

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

@AlanBateman
Copy link
Contributor

Most of the JVM TI tests for virtual threads are now failing on aarch64, they are hitting this assert

#  Internal Error (/workspace/open/src/hotspot/share/runtime/thread.hpp:480), pid=1448788, tid=1448821
#  assert(stack_base() > limit && limit >= stack_end()) failed: limit is outside of stack

:

V  [libjvm.so+0x17b1af0]  StackOverflow::enable_stack_reserved_zone(bool)+0x5c
V  [libjvm.so+0x174b688]  SharedRuntime::enable_stack_reserved_zone(JavaThread*)+0x44
j  jdk.internal.vm.Continuation.onContinue()V+0 java.base@20-ea
j  jdk.internal.vm.Continuation.yield0(Ljdk/internal/vm/ContinuationScope;Ljdk/internal/vm/Continuation;)Z+317 java.base@20-ea
j  jdk.internal.vm.Continuation.yield(Ljdk/internal/vm/ContinuationScope;)Z+69 java.base@20-ea
J 310 c1 java.lang.VirtualThread.yieldContinuation()Z java.base@20-ea (55 bytes) @ 0x0000ffff9d0503b8 [0x0000ffff9d050200+0x00000000000001b8]
J 155  jdk.internal.vm.Continuation.enterSpecial(Ljdk/internal/vm/Continuation;ZZ)V java.base@20-ea (0 bytes) @ 0x0000ffffa4aaf738 [0x0000ffffa4aaf6c0+0x0000000000000078]
J 326 c1 jdk.internal.vm.Continuation.run()V java.base@20-ea (586 bytes) @ 0x0000ffff9d05f8e8 [0x0000ffff9d05f040+0x00000000000008a8]
J 324 c1 java.lang.VirtualThread.runContinuation()V java.base@20-ea (135 bytes) @ 0x0000ffff9d05daf8 [0x0000ffff9d05d5c0+0x0000000000000538]
J 323 c1 java.lang.VirtualThread$$Lambda$8+0x000000080104b138.run()V java.base@20-ea (8 bytes) @ 0x0000ffff9d05d07c [0x0000ffff9d05cf80+0x00000000000000fc]
J 321 c1 java.util.concurrent.ForkJoinTask.doExec()I java.base@20-ea (37 bytes) @ 0x0000ffff9d05b094 [0x0000ffff9d05ae80+0x0000000000000214]
J 299 c1 java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Ljava/util/concurrent/ForkJoinTask;Ljava/util/concurrent/ForkJoinPool$WorkQueue;)V java.base@20-ea (83 bytes) @ 0x0000ffff9d046f0c [0x0000ffff9d046dc0+0x000000000000014c]
J 214 c1 java.util.concurrent.ForkJoinPool.scan(Ljava/util/concurrent/ForkJoinPool$WorkQueue;II)I java.base@20-ea (250 bytes) @ 0x0000ffff9d01beac [0x0000ffff9d01b840+0x000000000000066c]
j  java.util.concurrent.ForkJoinPool.runWorker(Ljava/util/concurrent/ForkJoinPool$WorkQueue;)V+35 java.base@20-ea
j  java.util.concurrent.ForkJoinWorkerThread.run()V+31 java.base@20-ea
v  ~StubRoutines::call_stub 0x0000ffffa45001bc
V  [libjvm.so+0xf8242c]  JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, JavaThread*)+0x5ac
V  [libjvm.so+0xf82a88]  JavaCalls::call_virtual(JavaValue*, Klass*, Symbol*, Symbol*, JavaCallArguments*, JavaThread*)+0x3e8
V  [libjvm.so+0xf82e04]  JavaCalls::call_virtual(JavaValue*, Handle, Klass*, Symbol*, Symbol*, JavaThread*)+0x70
V  [libjvm.so+0x10fbdb8]  thread_entry(JavaThread*, JavaThread*)+0x118
V  [libjvm.so+0xfb8bf4]  JavaThread::thread_main_inner()+0x250
V  [libjvm.so+0x18d0ac8]  Thread::call_run()+0xf8
V  [libjvm.so+0x15db2e4]  thread_native_entry(Thread*)+0x104
C  [libpthread.so.0+0x78f8]  start_thread+0x188

@theRealAph
Copy link
Contributor Author

Most of the JVM TI tests for virtual threads are now failing on aarch64, they are hitting this assert

OK, sorry. Can you tell me the name of one of them?

@AlanBateman
Copy link
Contributor

tier1 or run hotspot/jtreg:jdk_loom. I think most of the jdk/jdk_loom tests will fail too, but for other reasons.

@theRealAph
Copy link
Contributor Author

tier1 or run hotspot/jtreg:jdk_loom. I think most of the jdk/jdk_loom tests will fail too, but for other reasons.

OK, I think I know what that is. I'm on it.

@theRealAph
Copy link
Contributor Author

theRealAph commented Jul 4, 2022 via email

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
4 participants