8276314: [JVMCI] check alignment of call displacement during code installation #6218

dougxc · 2021-11-02T21:31:25Z

This PR add verification of code alignment invariants related to x64 call instructions during code installation.
This in turn allows a JVMCI compilation that generates a misaligned call to fail gracefully (i.e. bailout) instead of the VM crashing when it checks alignment before patching the displacement of a call instruction.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8276314: [JVMCI] check alignment of call displacement during code installation

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6218/head:pull/6218
$ git checkout pull/6218

Update a local copy of the PR:
$ git checkout pull/6218
$ git pull https://git.openjdk.java.net/jdk pull/6218/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6218

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6218.diff

bridgekeeper · 2021-11-02T21:32:57Z

👋 Welcome back dnsimon! 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 · 2021-11-02T21:34:53Z

@dougxc The following label will be automatically applied to this pull request:

hotspot-compiler

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.

mlbridge · 2021-11-02T21:39:31Z

Webrevs

vnkozlov · 2021-11-02T22:40:01Z

src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp

      call->set_destination(SharedRuntime::get_resolve_static_call_stub());
      _instructions->relocate(call->instruction_address(),
                                             relocInfo::static_call_type, Assembler::call32_operand);
      break;
    }
    case INVOKESPECIAL: {
      assert(method == NULL || !method->is_static(), "cannot call static method with invokespecial");
-      NativeCall* call = nativeCall_at(_instructions->start() + pc_offset);
+      call = nativeCall_at(_instructions->start() + pc_offset);
      call->set_destination(SharedRuntime::get_resolve_opt_virtual_call_stub());
      _instructions->relocate(call->instruction_address(),
                              relocInfo::opt_virtual_call_type, Assembler::call32_operand);
      break;
    }
    default:
      JVMCI_ERROR("invalid _next_call_type value");


May be print %d invalid call type here too since you are changing code around.

vnkozlov · 2021-11-02T22:41:47Z

src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp

-      break;
+      return;
+  }
+  if (os::is_MP() && !call->is_displacement_aligned()) {


You are checking for MP in current era? Why not always require alignment?

I agree. I think C2 has always aligned and C1 used to check is_MP but no longer does. Requiring JVMCI compilers to always align seems right, particularly since we don't expose is is_MP through JVMCI. Graal and C1 actually appear to over align the displacement by aligning it to BytesPerWord while C2 always aligns to 4 which is all that is required. It's odd that alignment check in NativeCall isn't simply displacement_offset() % 4. The existing check implies that it's ok to use a misaligned offset as long as it starts and ends within an 8 byte region but I don't know that that would really work and none of the compilers actually take advantage of it. That's probably beyond the scope of this PR. Actually verify_alignment checks that it's aligned on BytesPerInt so maybe is_displacement_aligned should unify around that definition.

I will remove the is_MP() calls.

while C2 always aligns to 4

I'm having trouble finding where that is done - can you please point it out.

It's deeply hidden. in x86_64.ad, it specifies the call alignment as 4 and in compute_padding it uses that alignment value to align the offset of the displacement.

Should we fix C1 and JVMCI to do the same as C2? As separate RFE.

There's nothing to fix on the JVMCI side other than ensuring that we're asserting the correct alignment restrictions. Reducing the alignment to 4 instead of 8 would be a pure Graal change.

It's deeply hidden

Thanks - doubt I would ever have found that in reasonable time.

I meant C1 and Graal. Yes, in JVMCI we should just check correct alignment.

enhanced error message for invalid _next_call_type value removed os::is_MP test

vnkozlov · 2021-11-03T22:05:19Z

/Users/runner/work/jdk/jdk/jdk/src/hotspot/cpu/x86/nativeInst_x86.hpp:180:103: error: no matching function for call to 'p2i'
  void  verify_alignment() { assert(is_displacement_aligned(), "displacement of call is not aligned", p2i()); }
                                                                                                      ^~~

vnkozlov

Good.
Please, run mach5 testing before push.

openjdk · 2021-11-03T23:04:39Z

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

8276314: [JVMCI] check alignment of call displacement during code installation

Reviewed-by: kvn

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

ce8c767: 8276220: Reduce excessive allocations in DateTimeFormatter
0ab910d: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
f3320d2: 8276588: Change "ccc" to "CSR" in HotSpot sources
32895ac: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
c7f070f: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
684edbb: 8273922: (fs) UserDefinedFileAttributeView doesn't handle file names that are just under the MAX_PATH limit (win)
7115892: 8276401: Use blessed modifier order in java.net.http
0ef8cbe: 8276385: Re-run blessed-modifier-order script on java.desktop and jdk.accessibility
724bf3b: 8275604: Zero: Reformat opclabels_data
61cb4bc: 8276036: The value of full_count in the message of insufficient codecache is wrong
... and 9 more: https://git.openjdk.java.net/jdk/compare/6a04899ba1a62f52f7e28cc2ed72bdca115e6562...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.

dougxc · 2021-11-18T08:32:01Z

/integrate

openjdk · 2021-11-18T08:33:04Z

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

9160743: 8276058: Some swing test fails on specific CI macos system
8193800: 8274179: AArch64: Support SVE operations with encodable immediates
b8453eb: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales
231fb61: 8276970: Default charset for PrintWriter that wraps PrintStream
29e552c: 8272358: Some tests may fail when executed with other locales than the US
ce4471f: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
45a60db: 8277045: G1: Remove unnecessary set_concurrency call in G1ConcurrentMark::weak_refs_work
6bb0462: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE
d8c0280: 8277316: ciReplay: dump_replay_data is not thread-safe
007ad7c: 8277303: Terminology mismatch between JLS17-3.9 and SE17's javax.lang.model.SourceVersion method specs
... and 202 more: https://git.openjdk.java.net/jdk/compare/6a04899ba1a62f52f7e28cc2ed72bdca115e6562...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-18T08:33:19Z

@dougxc Pushed as commit 2f4b540.

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

ensure call displacement is aligned during code installation

e147910

dougxc marked this pull request as ready for review November 2, 2021 21:33

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review labels Nov 2, 2021

vnkozlov reviewed Nov 2, 2021

View reviewed changes

consolidated verify_aligned with is_displacement_aligned

b21dc4e

enhanced error message for invalid _next_call_type value removed os::is_MP test

fix compilation error

807d20f

vnkozlov approved these changes Nov 3, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2021

openjdk bot closed this Nov 18, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 18, 2021

dougxc deleted the JDK-8276314 branch August 23, 2022 07:38

8276314: [JVMCI] check alignment of call displacement during code installation #6218

8276314: [JVMCI] check alignment of call displacement during code installation #6218

Uh oh!

Conversation

dougxc commented Nov 2, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 2, 2021

Uh oh!

openjdk bot commented Nov 2, 2021

Uh oh!

mlbridge bot commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Nov 18, 2021

Uh oh!

openjdk bot commented Nov 18, 2021

Uh oh!

openjdk bot commented Nov 18, 2021

Uh oh!

Uh oh!

dougxc commented Nov 2, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2021 •

edited

Loading

tkrodriguez Nov 2, 2021 •

edited

Loading

vnkozlov commented Nov 3, 2021 •

edited

Loading

openjdk bot commented Nov 3, 2021 •

edited

Loading