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

8289743: AArch64: Clean up patching logic #9398

Closed
wants to merge 36 commits into from

Conversation

theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Jul 6, 2022

The current logic for patching is a mess of if-then-elses. By rearranging the logic and using a switch we can make it both easier to understand and faster.


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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9398

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

Using diff file

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

@theRealAph theRealAph marked this pull request as draft July 6, 2022 13:28
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 6, 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 Jul 6, 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 Jul 6, 2022
@theRealAph theRealAph marked this pull request as ready for review July 7, 2022 13:11
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 7, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 7, 2022

@dholmes-ora
Copy link
Member

To be on the safe side I'm putting this through our internal testing. Please hold off integrating until I give it the green light. Thanks.

@theRealAph
Copy link
Contributor Author

To be on the safe side I'm putting this through our internal testing. Please hold off integrating until I give it the green light. Thanks.

OK, thanks. There's no hurry, and no need to get this one into the next release.

@dholmes-ora
Copy link
Member

dholmes-ora commented Jul 8, 2022

A lot of failures around one assertion AFAICS:

#  Internal Error (/opt/mach5/mesos/work_dir/slaves/0c72054a-24ab-4dbb-944f-97f9341a1b96-S10227/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/bfa54a47-3090-417b-b4b7-6433109e172e/runs/91d70c18-da15-420e-99f5-35e9f1ce15cb/workspace/open/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp:204), pid=844793, tid=844815
#  assert(target_addr_for_insn(insn_addr) == target) failed: should be
#
# JRE version: Java(TM) SE Runtime Environment (20.0) (fastdebug build 20-internal-2022-07-08-0655086.david.holmes.jdk-dev3.git)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 20-internal-2022-07-08-0655086.david.holmes.jdk-dev3.git, mixed mode, compressed class ptrs, z gc, linux-aarch64)
# Problematic frame:
# V  [libjvm.so+0x13fda54]  MacroAssembler::pd_patch_instruction_size(unsigned char*, unsigned char*)+0x114
#
---------------  T H R E A D  ---------------

Current thread (0x0000fffd502e3bc0):  JavaThread "C2 CompilerThread0" daemon [_thread_in_vm, id=844815, stack(0x0000fffd31c00000,0x0000fffd31e00000)]


Current CompileTask:
C2:    383   95    b        compiler.unsafe.UnsafeGetConstantField::checkGetAddress (10 bytes)

Stack: [0x0000fffd31c00000,0x0000fffd31e00000],  sp=0x0000fffd31dfa120,  free space=2024k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x13fda54]  MacroAssembler::pd_patch_instruction_size(unsigned char*, unsigned char*)+0x114
V  [libjvm.so+0x16febd0]  Relocation::pd_set_data_value(unsigned char*, long, bool)+0x40
V  [libjvm.so+0x16f8f5c]  external_word_Relocation::fix_relocation_after_move(CodeBuffer const*, CodeBuffer*)+0x8c
V  [libjvm.so+0xa3d444]  CodeBuffer::relocate_code_to(CodeBuffer*) const+0x480
V  [libjvm.so+0xa405c4]  CodeBuffer::copy_code_to(CodeBlob*)+0x90
V  [libjvm.so+0x155600c]  nmethod::nmethod(Method*, CompilerType, int, int, int, CodeOffsets*, int, DebugInformationRecorder*, Dependencies*, CodeBuffer*, int, OopMapSet*, ExceptionHandlerTable*, ImplicitExceptionTable*, AbstractCompiler*, int, char*, int, int)+0x3ec
V  [libjvm.so+0x1556640]  nmethod::new_nmethod(methodHandle const&, int, int, CodeOffsets*, int, DebugInformationRecorder*, Dependencies*, CodeBuffer*, int, OopMapSet*, ExceptionHandlerTable*, ImplicitExceptionTable*, AbstractCompiler*, int, char*, int, int, char const*, FailedSpeculation**)+0x270
V  [libjvm.so+0x94c144]  ciEnv::register_method(ciMethod*, int, CodeOffsets*, int, CodeBuffer*, int, OopMapSet*, ExceptionHandlerTable*, ImplicitExceptionTable*, AbstractCompiler*, bool, bool, bool, int, RTMState)+0x314
V  [libjvm.so+0x15fa92c]  PhaseOutput::install_code(ciMethod*, int, AbstractCompiler*, bool, bool, RTMState)+0x148
V  [libjvm.so+0xa8caac]  Compile::Code_Gen()+0x3fc
V  [libjvm.so+0xa9104c]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x112c
V  [libjvm.so+0x8c0918]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x1c4
V  [libjvm.so+0xa9f094]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x874
V  [libjvm.so+0xa9ff6c]  CompileBroker::compiler_thread_loop()+0x6ac
V  [libjvm.so+0xfb7dc4]  JavaThread::thread_main_inner()+0x250
V  [libjvm.so+0x18cfaa8]  Thread::call_run()+0xf8
V  [libjvm.so+0x15da2d4]  thread_native_entry(Thread*)+0x104
C  [libpthread.so.0+0x78f8]  start_thread+0x188

Different stacktraces.

@theRealAph
Copy link
Contributor Author

theRealAph commented Jul 8, 2022

OK, thanks. Any clue about a reproducer? I see it's zgc, and involves Unsafe, but these might not be requirements.

@dholmes-ora
Copy link
Member

There are failures in tier1, 2 and 3 (ZGC use is in tier3). Most failures on Macos.

Failing tests:

compiler/codecache/stress/RandomAllocationTest.java
compiler/codegen/TestOopCmp.java
compiler/unsafe/UnsafeGetConstantField.java
runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java#id4

@theRealAph
Copy link
Contributor Author

theRealAph commented Jul 11, 2022

There are failures in tier1, 2 and 3 (ZGC use is in tier3). Most failures on Macos.

Failing tests:

compiler/codecache/stress/RandomAllocationTest.java compiler/codegen/TestOopCmp.java compiler/unsafe/UnsafeGetConstantField.java runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java#id4

Got it. This is a corner case, triggered when we have a reloc of the form adrp; movk; add or adrp; movk, {ld,st}r[reg, #ofs].
It's a pre-existing bug which never has had any effect because this form is only ever used when the target is a fixed address. The assertion is new.

I'm running tier1 again now on MacOS.

@theRealAph theRealAph marked this pull request as ready for review July 18, 2022 16:09
@theRealAph
Copy link
Contributor Author

Clean tier1 linux-aarch64-server-fastdebug, macosx-aarch64-server-fastdebug.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 18, 2022
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.

Looks ok to me.

@openjdk
Copy link

openjdk bot commented Jul 18, 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:

8289743: AArch64: Clean up patching logic

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

  • 612d8c6: 8290456: remove os::print_statistics()
  • d7f0de2: 8290460: Alpine: disable some panama tests that rely on std::thread
  • dfbc691: 8289524: Add JFR JIT restart event
  • 4e6cd67: 8290000: Bump macOS GitHub actions to macOS 11
  • af86cd3: 8290463: Fix several comment typos in sun.security.ec
  • 6cd1c0c: Merge
  • 2677dd6: 8289954: C2: Assert failed in PhaseCFG::verify() after JDK-8183390
  • 4f3f74c: 8289127: Apache Lucene triggers: DEBUG MESSAGE: duplicated predicate failed which is impossible
  • 4a4d8ed: 8289801: [IR Framework] Add flags to whitelist which can be used to simulate a specific machine setup like UseAVX
  • 5a96a5d: 8289612: Change hotspot/jtreg tests to not use Thread.stop
  • ... and 182 more: https://git.openjdk.org/jdk/compare/77c3bbf105403089fec69d51406fe3e6f562271f...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 Jul 18, 2022
Copy link
Contributor

@nick-arm nick-arm left a comment

Choose a reason for hiding this comment

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

I ran tier1-3 without issue.

@theRealAph
Copy link
Contributor Author

I ran tier1-3 without issue.

Great, thanks for doing that.

@theRealAph
Copy link
Contributor Author

To be on the safe side I'm putting this through our internal testing. Please hold off integrating until I give it the green light. Thanks.

Would you like to do this again, please? It's been largely rewritten after review feedback.

@dholmes-ora
Copy link
Member

@theRealAph - our internal testing all passed: tiers 1-4

@theRealAph
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 20, 2022

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

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jul 20, 2022

@theRealAph Pushed as commit 1c05507.

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

@dean-long
Copy link
Member

We are seeing some new crashes after this change. The cause seems to be bad values using ADRP: see JDK-8290780.

@theRealAph
Copy link
Contributor Author

We are seeing some new crashes after this change. The cause seems to be bad values using ADRP: see JDK-8290780.

Oh! I thought that one had been tested fully.

@nick-arm
Copy link
Contributor

We are seeing some new crashes after this change. The cause seems to be bad values using ADRP: see JDK-8290780.

Is this test in the public repository? I can't find RunThese30M.java anywhere.

@dean-long
Copy link
Member

Is this test in the public repository? I can't find RunThese30M.java anywhere.

Unfortunately no, it's a closed Oracle test.

@theRealAph
Copy link
Contributor Author

We are seeing some new crashes after this change. The cause seems to be bad values using ADRP: see JDK-8290780.

I'm on it now. I think it might be a sign-extension bug when calculating the page offset.

@dean-long
Copy link
Member

offset = offset << (64-21) >> (64-21);

My guess is that ADRP is being used for a target address that is too far away. The added line above fixes up the high bits so that the assert in spatch() won't fire, hiding the problem:

239 guarantee (chk == -1 || chk == 0, "Field too big for insn");

@theRealAph
Copy link
Contributor Author

offset = offset << (64-21) >> (64-21);

My guess is that ADRP is being used for a target address that is too far away. The added line above fixes up the high bits so that the assert in spatch() won't fire, hiding the problem:

239 guarantee (chk == -1 || chk == 0, "Field too big for insn");

I've reproduced the problem and I'm now looking at the cleanest way to fix it.

@dean-long
Copy link
Member

I've reproduced the problem and I'm now looking at the cleanest way to fix it.

You can check out the patch I uploaded to JDK-8290780 and see if I'm on the right track on not :-)

@theRealAph
Copy link
Contributor Author

I've reproduced the problem and I'm now looking at the cleanest way to fix it.

You can check out the patch I uploaded to JDK-8290780 and see if I'm on the right track on not :-)

Yes, it's much the same. I fixed the verification logic too.

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