JDK-8320892: AArch64: Restore FPU control state after JNI #16851

theRealAph · 2023-11-28T14:26:11Z

Some buggy libraries corrupt the floating-point control register. Provide something similar to the x86 RestoreMXCSROnJNICalls.

I realize that using the x86ish name "RestoreMXCSROnJNICalls" might be a little controversial, but it is a global flag, not a CPU-specific one. And it's clearly intended for this purpose. It might have been better if that flag had been given a better name twentyish years ago, but we can't change it now.

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-8320892: AArch64: Restore FPU control state after JNI (Bug - P3)

Reviewers

Andrew Dinn (@adinn - Reviewer) ⚠️ Review applies to 02a7aaa0
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 02a7aaa0

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16851

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-28T14:27:49Z

👋 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 · 2023-11-28T14:29:05Z

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

mlbridge · 2023-11-28T14:32:30Z

Webrevs

adinn · 2023-11-28T15:09:19Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

+    // don't want non-IEEE rounding modes or floating-point traps.
+    bfi(tmp1, zr, 22, 4); // Clear DN, FZ, and Rmode
+    bfi(tmp1, zr, 8, 5);  // Clear exception-control bits (8-12)
+    eor(tmp1, tmp1, tmp2);


Hmm? So . . .

We ensure tmp1 has the bits we want by clearing DN FZ Rmode and Exception bits

we XOR tmp1 with the original bits (saved in tmp2) and put the 'difference' in tmp1 (?)

If the difference is zero we skip

Otherwise we write tmp1 (i.e. the 'difference' bits) to fpcr (???)

Should this not be

get_fpcr(tmp1); mov(tmp2, tmp1); bfi(tmp1, zr, 22, 4); bfi(tmp1, zr, 8, 5); eor(tmp2, tmp1, tmp2) cbz(tmp2, OK); set_fpcr(tmp1); bind(OK);

Argh, yes. The curse of the last-minute change...

Good catch.

adinn

Looks good.

openjdk · 2023-11-28T17:39:50Z

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

8320892: AArch64: Restore FPU control state after JNI

Reviewed-by: adinn, stuefe

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

78d0958: 8321406: Null IDs should be resolved as before catalogs are added
aaaae3e: 8321207: javac is not accepting correct code
86b27b7: 8317831: compiler/codecache/CheckLargePages.java fails on OL 8.8 with unexpected memory string
3cd65ce: 8321325: Remove unused Java_java_awt_MenuComponent_initIDs function
905137d: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
430564c: 8308715: Create a mechanism for Implicitly Declared Class javadoc
c8fa758: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS
9e57010: 8315149: Add hsperf counters for CPU time of internal GC threads
b0d1450: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted
acaf2c8: 8318590: JButton ignores margin when painting HTML text
... and 163 more: https://git.openjdk.org/jdk/compare/0c9a61c18545c7bd48e54e6b4e523b9ad8d0507d...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.

woess · 2023-11-29T14:26:30Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

+    bfi(tmp1, zr, 22, 4); // Clear DN, FZ, and Rmode
+    bfi(tmp1, zr, 8, 5);  // Clear exception-control bits (8-12)
+    eor(tmp2, tmp1, tmp2);
+    cbz(tmp2, OK);        // Only reset FPCR if it's wrong


should we maybe do the same in generate_call_stub, too (likely faster)?

Sure, it would be. I didn't think it was worthwhile, but we have the code now.

tkrodriguez · 2023-11-29T16:09:11Z

x86 also has a restore call in DowncallLinker::StubGenerator::generate so you might consider adding there as well.

tkrodriguez · 2023-11-29T16:14:40Z

Regarding the flag name you could introduce a better name and alias to it RestoreMXCSROnJNICalls. Reusing the same name it definitely confusing. At a minimum you need to update the documentation for RestoreMXCSROnJNICalls to indicate that it also does something on aarch64 or generalize the language to something like Restore floating point control word when returning from JNI calls.

theRealAph · 2023-11-29T16:36:10Z

Regarding the flag name you could introduce a better name and alias to it RestoreMXCSROnJNICalls.

Ah, I did not know that flag aliases were possible. I'll have a look at something generic.

Reusing the same name it definitely confusing. At a minimum you need to update the documentation for RestoreMXCSROnJNICalls to indicate that it also does something on aarch64 or generalize the language to something like Restore floating point control word when returning from JNI calls.

I'll go digging.

theRealAph · 2023-11-29T16:36:32Z

x86 also has a restore call in DowncallLinker::StubGenerator::generate so you might consider adding there as well.

I think it's there.

tkrodriguez · 2023-11-29T18:28:57Z

Yes it's there. Sorry I guess I was scanning too quickly.

dholmes-ora · 2023-11-30T05:42:57Z

I was also going to suggest adding a new flag and creating an alias. The new flag will need a CSR request of course.

theRealAph · 2023-11-30T09:58:28Z

I was also going to suggest adding a new flag and creating an alias. The new flag will need a CSR request of course.

Given that it's new and it's diagnostic flag I'm a bit surprised at that. I was trying for a quick fix.

Anyway, how do you create an alias? I can't see any examples, and I haven't found a way through the maze of twisty #define passages.

dholmes-ora · 2023-12-04T04:52:47Z

@theRealAph the RestoreMXCSROnJNICalls flag is a product flag not diagnostic.

Aliased flags are setup in arguments.cpp by editing this:

static AliasedFlag const aliased_jvm_flags[] = {
  { "DefaultMaxRAMFraction",    "MaxRAMFraction"    },
  { "CreateMinidumpOnCrash",    "CreateCoredumpOnCrash" },
  { nullptr, nullptr}
};

dholmes-ora · 2023-12-04T04:54:53Z

Also from arguments.cpp

 *      ALIASED: An option that is simply another name for another option. This is often
 *               part of the process of deprecating a flag, but not all aliases need
 *               to be deprecated.
 *
 *               Create an alias for an option by adding the old and new option names to the
 *               "aliased_jvm_flags" table. Delete the old variable from globals.hpp (etc).

theRealAph · 2023-12-04T15:43:23Z

@theRealAph the RestoreMXCSROnJNICalls flag is a product flag not diagnostic.

Ah, thanks,

Aliased flags are setup in arguments.cpp by editing this:

OK. How about we split this into two, this first part without a CSR, and the second part, which creates the generic alias, with one? That way we can mitigate a live problem in this release.

theRealAph · 2023-12-05T09:44:24Z

@theRealAph the RestoreMXCSROnJNICalls flag is a product flag not diagnostic.

Ah, thanks,

Aliased flags are setup in arguments.cpp by editing this:

OK. How about we split this into two, this first part without a CSR, and the second part, which creates the generic alias, with one? That way we can mitigate a live problem in this release.

Please? One day left.

tstuefe

Looks good.

I looked at https://help.totalview.io/previous_releases/2019/HTML/index.html#page/Reference_Guide%2FPowerFPSCRRegister_2.html%23, and there seems to be another exception bit, bit 15, the "Input denormal exception enable". That one is okay to be modified?

teshull · 2023-12-05T11:37:48Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

+    // Set FPCR to the state we need. We do want Round to Nearest. We
+    // don't want non-IEEE rounding modes or floating-point traps.
+    bfi(tmp1, zr, 22, 4); // Clear DN, FZ, and Rmode
+    bfi(tmp1, zr, 8, 5);  // Clear exception-control bits (8-12)


(Related to both this PR and #16637)

Shouldn't also explicit flushing inputs to zero, i.e. when AH:FIZ is (1:1), be protected against?

Also, is it necessary to clear DN? When looking at the spec, I think this should be allowed.

(Related to both this PR and #16637)

Shouldn't also explicit flushing inputs to zero, i.e. when AH:FIZ is (1:1), be protected against?

I've avoided touching ArmV8.7 ALT_FP, but you might be right. AH:FIZ are both RES0, so it is safe to do so.

Also, is it necessary to clear DN? When looking at the spec, I think this should be allowed.

I think it's wise to clear DN. We save and restore all of the FPCR at entry to Java, clearing DN, and it's not unreasonable to expect a JNI call not to mess with FPCR. Also, I think replacing NaN payload bits with the default NaN is pointless, and violates the principle of least surprise if not the spec of longBitsToDouble.

dholmes-ora · 2023-12-06T04:57:50Z

OK. How about we split this into two, this first part without a CSR, and the second part, which creates the generic alias, with one? That way we can mitigate a live problem in this release.

I'm fine with splitting it if needed, but I also re-evaluated this as a P3 so it can still go into 22 after RDP 1.

theRealAph · 2023-12-06T15:31:39Z

/integrate

openjdk · 2023-12-06T15:32:37Z

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

0217b5a: 8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API
7fbfb3b: 8321369: Unproblemlist gc/cslocker/TestCSLocker.java
2678e4c: 8319111: Mismatched MemorySegment heap access is not consistently intrinsified
a0920aa: 8321159: SymbolLookup.libraryLookup(Path, Arena) Assumes default Filesystem
9d77677: 8321124: java/util/stream/GatherersTest.java times out
4c96aac: 8320935: Move CDS config initialization code to cdsConfig.cpp
78d0958: 8321406: Null IDs should be resolved as before catalogs are added
aaaae3e: 8321207: javac is not accepting correct code
86b27b7: 8317831: compiler/codecache/CheckLargePages.java fails on OL 8.8 with unexpected memory string
3cd65ce: 8321325: Remove unused Java_java_awt_MenuComponent_initIDs function
... and 169 more: https://git.openjdk.org/jdk/compare/0c9a61c18545c7bd48e54e6b4e523b9ad8d0507d...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-06T15:32:47Z

@theRealAph Pushed as commit 50f3124.

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

robehn · 2024-04-19T09:00:14Z

@theRealAph IMHO we should add a new flag aliasing the old. Then deprecate MXCSR flag.

JDK-8320892: AArch64: Restore FPU control state after JNI

8dc2033

openjdk bot added the rfr Pull request is ready for review label Nov 28, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 28, 2023

Fix oop map

590bb9a

adinn reviewed Nov 28, 2023

View reviewed changes

Fix thinko

02a7aaa

adinn approved these changes Nov 28, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 28, 2023

woess reviewed Nov 29, 2023

View reviewed changes

tstuefe approved these changes Dec 5, 2023

View reviewed changes

teshull reviewed Dec 5, 2023

View reviewed changes

Clear AH:FIZ

d2afa41

openjdk bot added the integrated Pull request has been integrated label Dec 6, 2023

openjdk bot closed this Dec 6, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8320892: AArch64: Restore FPU control state after JNI #16851

JDK-8320892: AArch64: Restore FPU control state after JNI #16851

theRealAph commented Nov 28, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 28, 2023

openjdk bot commented Nov 28, 2023

mlbridge bot commented Nov 28, 2023 •

edited

Loading

adinn Nov 28, 2023

theRealAph Nov 28, 2023

adinn left a comment

openjdk bot commented Nov 28, 2023 •

edited

Loading

woess Nov 29, 2023

theRealAph Nov 29, 2023

tkrodriguez commented Nov 29, 2023

tkrodriguez commented Nov 29, 2023

theRealAph commented Nov 29, 2023

theRealAph commented Nov 29, 2023

tkrodriguez commented Nov 29, 2023

dholmes-ora commented Nov 30, 2023

theRealAph commented Nov 30, 2023

dholmes-ora commented Dec 4, 2023

dholmes-ora commented Dec 4, 2023

theRealAph commented Dec 4, 2023

theRealAph commented Dec 5, 2023

tstuefe left a comment

teshull Dec 5, 2023

theRealAph Dec 5, 2023

dholmes-ora commented Dec 6, 2023

theRealAph commented Dec 6, 2023

openjdk bot commented Dec 6, 2023

openjdk bot commented Dec 6, 2023

robehn commented Apr 19, 2024

JDK-8320892: AArch64: Restore FPU control state after JNI #16851

JDK-8320892: AArch64: Restore FPU control state after JNI #16851

Conversation

theRealAph commented Nov 28, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 28, 2023

openjdk bot commented Nov 28, 2023

mlbridge bot commented Nov 28, 2023 • edited Loading

Webrevs

adinn Nov 28, 2023

Choose a reason for hiding this comment

theRealAph Nov 28, 2023

Choose a reason for hiding this comment

adinn left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 28, 2023 • edited Loading

woess Nov 29, 2023

Choose a reason for hiding this comment

theRealAph Nov 29, 2023

Choose a reason for hiding this comment

tkrodriguez commented Nov 29, 2023

tkrodriguez commented Nov 29, 2023

theRealAph commented Nov 29, 2023

theRealAph commented Nov 29, 2023

tkrodriguez commented Nov 29, 2023

dholmes-ora commented Nov 30, 2023

theRealAph commented Nov 30, 2023

dholmes-ora commented Dec 4, 2023

dholmes-ora commented Dec 4, 2023

theRealAph commented Dec 4, 2023

theRealAph commented Dec 5, 2023

tstuefe left a comment

Choose a reason for hiding this comment

teshull Dec 5, 2023

Choose a reason for hiding this comment

theRealAph Dec 5, 2023

Choose a reason for hiding this comment

dholmes-ora commented Dec 6, 2023

theRealAph commented Dec 6, 2023

openjdk bot commented Dec 6, 2023

openjdk bot commented Dec 6, 2023

robehn commented Apr 19, 2024

theRealAph commented Nov 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 28, 2023 •

edited

Loading

openjdk bot commented Nov 28, 2023 •

edited

Loading