8291752: AArch64: Remove check_emit_size parameter from trampoline_call #9782

eastig · 2022-08-05T16:59:08Z

MacroAssembler::trampoline_call can skip generation of a trampoline stub if C2 phase output is in the checking emitted code size mode. If it is not C2 compilation, trampoline_call always generates a trampoline stub.

gen_continuation_enter as a part of C2 compilation needed to disable the functionality above. Now trampoline_call has the check_emit_size which allows to disable the functionality completely. Its default value is true which means the functionality is enabled.

There are problems:

C2 specific code which must be properly guarded if it is not C2 compilation. The code has the risk of undefined behaviour (UB). It is done with is_c2_compile. JDK-8291654 is an example of changes caused UB.
The default value of check_emit_size. The current value true allows C1 to access the C2 specific code. Setting the default value to false will require to review and to update every existing use of trampoline_call. It will complicate uses of trampoline_call: each time a decision needs to be done - use or not use the default.

Summary of changes:

Have a virtual in_scratch_emit_size returning false by default. It is privately overridden in C2_MacroAssembler. The overridden version returns true if C2 phase output is in the checking emitted code size mode.
check_emit_size is removed because it is not needed any more.

Tested with fastdebug/release builds:

gtest: Passed.
tier1...tier4: Passed.

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-8291752: AArch64: Remove check_emit_size parameter from trampoline_call

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 2e5e4aee
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9782

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

Using diff file

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

bridgekeeper · 2022-08-05T17:00:44Z

👋 Welcome back eastig! 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 · 2022-08-05T17:05:15Z

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

eastig · 2022-08-05T17:06:22Z

/label hotspot-compiler

openjdk · 2022-08-05T17:07:04Z

@eastig
The hotspot-compiler label was successfully added.

mlbridge · 2022-08-05T17:10:09Z

Webrevs

vnkozlov

I have few comments.

src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

vnkozlov · 2022-08-05T17:53:28Z

src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp

+  // If we generated no stub, patch this call directly to dest.
+  // This will happen if we don't need far branches or if there
+  // already was a trampoline.


I think this comment should be changed since you changed the code.

The original code looks strange to me.
If we need a stub but we have failed to create it, we set dest anyway. This will lead to an incorrect instruction.
The behaviour is undefined. We might fail executing the instruction or jump somewhere.

I've restored the original functionality.
Maybe Andrew(@theRealAph) can explain what should be here.

vnkozlov

Good.

openjdk · 2022-08-05T20:25:01Z

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

8291752: AArch64: Remove check_emit_size parameter from trampoline_call

Reviewed-by: kvn, aph

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

ecfa38f: 8281966: Absolute path of symlink is null in JFileChooser
83dc2e3: 8292062: misc javax/swing tests failing
4913380: 8292007: Do not include vmSymbol.hpp in method.hpp
6397d56: 8151430: (fs) BasicFileAttributeView.setTimes should support setting file create time on OS X
57e0da1: 8292132: ProblemList jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
17c77b5: 8292119: ProblemList java/awt/image/multiresolution/MultiresolutionIconTest.java on linux-x64 and windows-all
d889264: 8290836: Improve test coverage for XPath functions: String Functions
ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
8d0d9ea: 8291238: JDK can't be built without G1
... and 75 more: https://git.openjdk.org/jdk/compare/0971d3464609bf4124df460ea73ff761d7e0f7b2...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 (@vnkozlov, @theRealAph) 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).

eastig · 2022-08-05T20:26:22Z

Good.

@vnkozlov Thank you for reviewing.

theRealAph · 2022-08-08T10:33:15Z

This looks very wrong to me. We could usefully add trampolines to C1, and this just makes doing that more difficult.

Why don't you just define is_c2_compilation() appropriately in macroAssembler?

theRealAph · 2022-08-08T10:37:32Z

Why don't you just define is_c2_compilation() appropriately in macroAssembler?

Or (better) in_scratch_emit_size()? That's the semantics we want.

eastig · 2022-08-08T15:19:32Z

Why don't you just define is_c2_compilation() appropriately in macroAssembler?

Or (better) in_scratch_emit_size()? That's the semantics we want.

Thank you for the suggestion. It's better.
I rewrote the code to use in_scratch_emit_size().

eastig · 2022-08-09T20:21:17Z

@vnkozlov @theRealAph, any comments on the version with the virtual in_scratch_emit_size()?

eastig · 2022-08-10T12:50:10Z

@theRealAph Thank you

eastig · 2022-08-10T12:50:48Z

/integrate

openjdk · 2022-08-10T12:52:31Z

@eastig
Your change (at version 71a1ef0) is now ready to be sponsored by a Committer.

simonis · 2022-08-10T14:54:49Z

/sponsor

openjdk · 2022-08-10T14:56:28Z

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

37d3146: 8289002: Minimal x86_64 VM build fails with GCC 11: 'this' pointer is null
ecfa38f: 8281966: Absolute path of symlink is null in JFileChooser
83dc2e3: 8292062: misc javax/swing tests failing
4913380: 8292007: Do not include vmSymbol.hpp in method.hpp
6397d56: 8151430: (fs) BasicFileAttributeView.setTimes should support setting file create time on OS X
57e0da1: 8292132: ProblemList jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
17c77b5: 8292119: ProblemList java/awt/image/multiresolution/MultiresolutionIconTest.java on linux-x64 and windows-all
d889264: 8290836: Improve test coverage for XPath functions: String Functions
ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
... and 76 more: https://git.openjdk.org/jdk/compare/0971d3464609bf4124df460ea73ff761d7e0f7b2...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-10T14:56:47Z

@simonis @eastig Pushed as commit cb37282.

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

8291752: AArch64: Remove check_emit_size parameter from trampoline_call

ab81d0b

openjdk bot added the rfr Pull request is ready for review label Aug 5, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 5, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Aug 5, 2022

eastig mentioned this pull request Aug 5, 2022

8291654: AArch64: assert from JDK-8287393 causes crashes #9709

Closed

3 tasks

vnkozlov reviewed Aug 5, 2022

View reviewed changes

eastig added 3 commits August 5, 2022 19:59

Remove unneeded assert

eb1d2a1

Substitute entry.target() with target

64cef26

Restore original NativeCall::trampoline_jump functionality

2e5e4ae

vnkozlov approved these changes Aug 5, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 5, 2022

Add virtual in_scratch_emit_size instead of emit_trampoline_stub

71a1ef0

theRealAph approved these changes Aug 10, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 10, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 10, 2022

openjdk bot closed this Aug 10, 2022

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 Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8291752: AArch64: Remove check_emit_size parameter from trampoline_call #9782

8291752: AArch64: Remove check_emit_size parameter from trampoline_call #9782

eastig commented Aug 5, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 5, 2022

openjdk bot commented Aug 5, 2022

eastig commented Aug 5, 2022

openjdk bot commented Aug 5, 2022

mlbridge bot commented Aug 5, 2022 •

edited

Loading

vnkozlov left a comment

vnkozlov Aug 5, 2022

eastig Aug 5, 2022

eastig Aug 5, 2022

vnkozlov left a comment

openjdk bot commented Aug 5, 2022 •

edited

Loading

eastig commented Aug 5, 2022

theRealAph commented Aug 8, 2022

theRealAph commented Aug 8, 2022

eastig commented Aug 8, 2022

eastig commented Aug 9, 2022

eastig commented Aug 10, 2022

eastig commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

simonis commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

8291752: AArch64: Remove check_emit_size parameter from trampoline_call #9782

8291752: AArch64: Remove check_emit_size parameter from trampoline_call #9782

Conversation

eastig commented Aug 5, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 5, 2022

openjdk bot commented Aug 5, 2022

eastig commented Aug 5, 2022

openjdk bot commented Aug 5, 2022

mlbridge bot commented Aug 5, 2022 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Aug 5, 2022

Choose a reason for hiding this comment

eastig Aug 5, 2022

Choose a reason for hiding this comment

eastig Aug 5, 2022

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 5, 2022 • edited Loading

eastig commented Aug 5, 2022

theRealAph commented Aug 8, 2022

theRealAph commented Aug 8, 2022

eastig commented Aug 8, 2022

eastig commented Aug 9, 2022

eastig commented Aug 10, 2022

eastig commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

simonis commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

openjdk bot commented Aug 10, 2022

eastig commented Aug 5, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 5, 2022 •

edited

Loading

openjdk bot commented Aug 5, 2022 •

edited

Loading