8342769: HotSpot Windows/gcc port is broken #21627

TheShermanTanker · 2024-10-22T01:17:01Z

Several areas in HotSpot are broken in the gcc port. These, with the exception of 1 rather big oversight within SharedRuntime::frem and SharedRuntime::drem, are all minor correctness issues within the code. These mostly can be fixed with simple changes to the code. Note that I am not sure whether the SharedRuntime::frem and SharedRuntime::drem fix is correct. It may be that they can be removed entirely

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-8342769: HotSpot Windows/gcc port is broken (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Andrew Haley (@theRealAph - Reviewer) Review applies to 6cf1c83a

Reviewers without OpenJDK IDs

@swesonga (no known openjdk.org user name / role) Review applies to 6cf1c83a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21627

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-22T01:17:36Z

👋 Welcome back jwaters! 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 · 2024-10-22T01:18:17Z

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

8342769: HotSpot Windows/gcc port is broken

Reviewed-by: dholmes, 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 105 new commits pushed to the master branch:

18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
31c3b19: 8346174: UMAX/UMIN are missing from XXXVector::reductionOperations
c75b1d4: 8346082: Output JVMTI agent information in hserr files
51662c2: 8346232: Remove leftovers of the jar --normalize feature
d335941: 8345678: compute_modifiers should not be in create_mirror
57adf64: 8346261: Cleanup in JDP tests
... and 95 more: https://git.openjdk.org/jdk/compare/a606836a02bb3e4ff3e3aaad478fc7ac142b3e38...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 · 2024-10-22T01:18:56Z

@TheShermanTanker 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 · 2024-10-22T01:22:26Z

Webrevs

dholmes-ora

The sharedRuntime stuff is tricky and messy. Not sure about what is best/right/workable there. Maybe we even need to factor out like ./os/windows/sharedRuntimeRem.cpp?

dholmes-ora · 2024-10-22T02:02:54Z

src/hotspot/os/windows/memMapPrinter_windows.cpp

-    st->print("%#11llx", reinterpret_cast<const unsigned long long>(mem_info.BaseAddress) - reinterpret_cast<const unsigned long long>(mem_info.AllocationBase));
+    st->print("%#11llx", reinterpret_cast<unsigned long long>(mem_info.BaseAddress) - reinterpret_cast<unsigned long long>(mem_info.AllocationBase));


What is the issue with const here?

More a correctness issue. The const is not required since the cast result is an rvalue unsigned long long. It's similar to something like a method's return type being marked as const int get(); where the const doesn't really mean anything since there is no possible way for it to be modified

dholmes-ora · 2024-10-22T02:03:29Z

src/hotspot/cpu/x86/sharedRuntime_x86.cpp

+#ifdef _WIN64
+const juint  float_sign_mask  = 0x7FFFFFFF;
+const juint  float_infinity   = 0x7F800000;
+const julong double_sign_mask = CONST64(0x7FFFFFFFFFFFFFFF);
+const julong double_infinity  = CONST64(0x7FF0000000000000);
+#endif


Isn't all this better suited for sharedRuntime_x86_64.cpp?

You mean the entire custom frem and drem implementation unique to x64 Windows, or just this block of constants here? These constants are used by the custom Windows implementation, so they have to be in the same file. Moving the entire definition for Windows x64 to sharedRuntime_x86_64.cpp seems to be duplicating code even more, and I still don't know whether this is the right solution. It may be that this custom implementation may not be needed anymore and should be removed, which is why I left it in the sharedRuntime_x86.cpp file for the time being

I did mean the entire thing. I don't see any point in WIN64 code being in the x86 file when there is an x86_64 file. Of course I had forgotten about the Aarch64 Windows side, so maybe as I said this is really something for a os/windows file rather then cpu/<arch> ?

If it's needed on AArch64 too, this should move to something in os/windows as David said.

However, judging from the comment in

jdk/src/hotspot/os/windows/sharedRuntimeRem.cpp

Lines 34 to 38 in c40bb76

// This code is a copy of __ieee754_fmod() formerly from the JDK's libfdlibm and is

// used as a workaround for issues with the Windows x64 CRT implementation

// of fmod. Microsoft has acknowledged that this is an issue in Visual Studio

// 2012 and forward, but has not provided a time frame for a fix other than that

// it'll not be fixed in Visual Studio 2013 or 2015.

this workaround might not be needed anymore. The minimum version required for Windows-AArch64 is VS2017 (or even VS2019?), and for Windows-X64 apparently VS2022 is used (not sure what the minimum is):

jdk/make/conf/jib-profiles.js

Line 1105 in c40bb76

windows_x64: "VS2022-17.6.5+1.0",

So maybe this can be removed altogether? Someone needs to check if this assumption is still true today.

Alas I don't have a Windows setup anymore. @karianna who is the local HotSpot Windows expert these days who could have a look at this? 🙂

I don't think this workaround was intended for ARM64 in the first place, it explicitly mentions that it was written for x64, and the ifdef mess that resulted in this bug ended up bringing it over to ARM64

The minimum Visual Studio version required to build the Windows AArch64 port is VS 2022 via the DevKit - https://wiki.openjdk.org/display/Build/Supported+Build+Platforms

I'm now verifying whether this workaround is still needed.

@TheShermanTanker - this workaround is still needed on Windows AArch64. I haved filed https://developercommunity.visualstudio.com/t/fmod-incorrectly-returns-NaN-on-certain-/10793176 to track a fix for the underlying fmod bug.

I'm astounded by how this happy accident looped all the way back around to being needed for ARM64, truly one of the funniest coincidences of all time

I don't think this workaround was intended for ARM64 in the first place, it explicitly mentions that it was written for x64, and the ifdef mess that resulted in this bug ended up bringing it over to ARM64

So what? I'm sure it'll be fine on AArch64. Just use it.

I don't think this workaround was intended for ARM64 in the first place, it explicitly mentions that it was written for x64, and the ifdef mess that resulted in this bug ended up bringing it over to ARM64

So what? I'm sure it'll be fine on AArch64. Just use it.

This comment was made before Saint Wesonga confirmed that Windows/ARM64 now needed the workaround

TheShermanTanker · 2024-10-22T03:49:54Z

The sharedRuntime stuff is tricky and messy. Not sure about what is best/right/workable there. Maybe we even need to factor out like ./os/windows/sharedRuntimeRem.cpp?

I'm not sure what is best too. I think we might need more people looking at this, in particular Windows ARM64 maintainers, because they might be the only platform that this change will affect. Some in the mailing lists mentioned that fmod_winx64 might not be needed anymore and could be removed entirely

TheShermanTanker · 2024-10-30T05:26:14Z

Bumping, please advise how to proceed on the sharedRuntime issue. It potentially affects Windows/ARM64 in an unknown way

dholmes-ora · 2024-10-30T05:49:40Z

You need to contact the Windows-Aarch64 port maintainers and get them to tell you what the correct thing for Aarch64 is. Paging @mo-beck , @luhenry and @lewurm (https://openjdk.org/jeps/388)

karianna · 2024-11-02T04:01:43Z

Passing onto Microsoft's VM Engineering manager @brianjstafford - he'll find someone to take a look and advise.

brianjstafford · 2024-11-04T18:43:58Z

@swesonga, can you please take a look at this?

TheShermanTanker · 2024-11-08T09:41:38Z

Bumping

swesonga · 2024-11-08T16:05:41Z

Bumping

Reviewing this change today. cc @brianjstafford

TheShermanTanker · 2024-11-13T06:47:19Z

Bumping

brianjstafford · 2024-11-13T17:17:17Z

@swesonga is out of office this week and will resume looking into this when he's back.

TheShermanTanker · 2024-11-20T08:43:28Z

I've changed the check to instead define the constants when _M_ARM64 is defined. This is the cleanest approach I can come up with, and doesn't touch the workaround code in frem and drem either

…indows.cpp

theRealAph · 2024-11-20T09:04:49Z

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

TheShermanTanker · 2024-11-20T09:16:02Z

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

theRealAph · 2024-11-20T10:56:29Z

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Oh, OK. I'll have a look.

theRealAph · 2024-11-20T12:17:30Z

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Oh, OK. I'll have a look.

I had a look at SharedRuntime::fmod_winx64. I'm sure it's fine for AArch64. Just put it in os_windows.

TheShermanTanker · 2024-11-25T10:13:47Z

Bump

TheShermanTanker · 2024-12-02T12:01:17Z

Bumping

karianna · 2024-12-03T06:48:58Z

@TheShermanTanker - Is there someone in particular you're waiting to approve this PR? I think you can re-request reviews.

TheShermanTanker · 2024-12-03T07:11:45Z

@karianna I need 2 reviews with at least 1 of them being from a reviewer before this is marked by the automated systems as allowed for integration - Skara doesn't count swesonga's review because he's apparently not registered in the census (I think there might be a mistake there, either in the Skara tool or someone forgot to register him). I guess David or Andrew, or any one of the people from Microsoft who have seen this PR are the next best people to approve this. I had forgotten completely about the review request feature, thanks for reminding me :)

theRealAph

Looks good.

dholmes-ora

So for the sharedRuntime changes, IIUC what was previously only needed for Windows x64 is now only needed for Windows Aarch64 - correct?

dholmes-ora · 2024-12-04T01:19:57Z

src/hotspot/os/windows/sharedRuntimeRem.cpp

@@ -25,6 +25,7 @@
 #include "precompiled.hpp"
 #include "runtime/sharedRuntime.hpp"

+#ifdef _M_ARM64


As this is ARM64 only now, the comment block at line 34 needs updating

dholmes-ora · 2024-12-04T01:21:56Z

src/hotspot/share/runtime/sharedRuntime.cpp

@@ -284,7 +284,7 @@ JRT_LEAF(jlong, SharedRuntime::lrem(jlong y, jlong x))
 JRT_END


-#ifdef _WIN64
+#ifdef _M_ARM64


If this is ARM64 only then the #if !defined(X86) at line 294 is not needed.

Wouldn't this cause a multiple definition error for frem and drem if the #if !defined(X86) was removed? The #if prevents those 2 methods from being defined when on x86 so it doesn't conflict with the x86 specific definitions, without it frem and drem would be defined twice on x86

But if we enter this block because _M_ARM64 is defined then X86 cannot be defined so line 294 is vacuously true.

I'm not sure I follow - The _M_ARM64 block is terminated by an endif before line 294, it is only there to define the global constants and does not guard the code below that line 294 does. Additionally, the code guarded by the if X86 seems to be for all non x86 platforms, not just ARM64

Apologies somehow the endif at line 292 was invisible

I'm not really sure what you mean, if you mean why the _M_ARM64 ifdef doesn't guard the frem and drem implementations in sharedRuntime.cpp here, it's because the Windows/ARM64 implementation with the workaround in it and the implementation for the rest of the platforms (minus x86) are squashed right next to each other. Which implementation is compiled into HotSpot is then determined by the #ifdef _WIN64 check on line 296. If the _M_ARM64 ifdef covered the frem and drem implementation here then the implementation for all non Windows/ARM64 and non x86 platforms would not be defined, and HotSpot wouldn't link at all. Well, that's if I understood you correctly, at least

It might be possible to split the Windows/ARM64 implementation specifically into another file to reduce the confusion, but I feel like that is more trouble than it's worth, and may just cause more issues if not done right. I'd prefer not to do that to avoid all those problems

Sorry I'm doing a really bad job of pretending to be the preprocessor. :)

So the block at line 287 defines some constants for all ARM64, even though they are only needed on Windows.

The block at line 294 provides function definitions for all CPUs other the x86. Then within that we have one version for WIN64 (which really means Windows/Aarch64) and a regular version for all other non-x86 CPUS and non-Windows Aarch64.

So I guess what I'm suggesting is that it would be clearer to use if defined(_M_ARM64) && defined(WINDOWS) at lines 287, 294 and 321, to be clear the special code is only for Windows/Aarch64.

Second I think it would be clearer to just have two blocks: one for windows/aarch64 and the other for all other non-x86, rather than interleaving things the way they have been done.

Ah, I see what you mean. There's just a small change I would make, which is to check whether defined(_WINDOWS) and defined(AARCH64) instead. Checking for both _M_ARM64 and _WINDOWS is redundant; _M_ARM64 is a Windows only define, so checking for _M_ARM64 == checking for AARCH64 && _WINDOWS. Would you be ok with that instead?

I think(?) I get what you mean by having 2 blocks, I'll try to do that

Sorry I'm doing a really bad job of pretending to be the preprocessor. :)

Haha, it happens

Ah, I see what you mean. There's just a small change I would make, which is to check whether defined(_WINDOWS) and defined(AARCH64) instead. Checking for both _M_ARM64 and _WINDOWS is redundant; _M_ARM64 is a Windows only define, so checking for _M_ARM64 == checking for AARCH64 && _WINDOWS.

Checking for AARCH64 && _WINDOWS is much easier to understand.

Andrew and David, are the latest changes what you had in mind?

dholmes-ora · 2024-12-04T01:24:46Z

src/hotspot/os/windows/sharedRuntimeRem.cpp

-double SharedRuntime::fmod_winx64(double x, double y)
+double SharedRuntime::fmod_win64(double x, double y)


Wouldn't fmod_winAarch64 or fmod_winARM64 make more sense given this is now only used for Aarch64?

The name was suggested by Andrew, I'll change it to fmod_winarm64 if you prefer

I thought at the time the name was because it was for both x64 and aarch64 - hence any win64 platform. But if it is only for Aarch64 then it makes sense to me that the function name conveys that.

Alright, I'll change it

dholmes-ora · 2024-12-04T01:27:57Z

I had forgotten completely about the review request feature, thanks for reminding me :)

Only works if you have Github notifications enabled (I don't) else you have to return to the PR to see it.

TheShermanTanker · 2024-12-04T04:13:29Z

So for the sharedRuntime changes, IIUC what was previously only needed for Windows x64 is now only needed for Windows Aarch64 - correct?

Yes, that seems to be the case

dholmes-ora

That seems a lot clearer to me now - thanks.

Just one query below.

dholmes-ora · 2024-12-10T05:13:06Z

src/hotspot/os/windows/sharedRuntimeRem.cpp

@@ -25,7 +25,7 @@
 #include "precompiled.hpp"
 #include "runtime/sharedRuntime.hpp"

-#ifdef _M_ARM64
+#ifdef AARCH64


Shouldn't this also be

#if defined(_WINDOWS) && defined(AARCH64)

?

sharedRuntimeRem.cpp is a Windows only file - Checking for _WINDOWS for a file under os/windows is somewhat redundant

Sorry, missed that. Please ignore.

dholmes-ora · 2024-12-10T07:51:11Z

src/hotspot/os/windows/sharedRuntimeRem.cpp

@@ -25,7 +25,7 @@
 #include "precompiled.hpp"
 #include "runtime/sharedRuntime.hpp"

-#ifdef _M_ARM64
+#ifdef AARCH64


Sorry, missed that. Please ignore.

bridgekeeper · 2025-01-07T08:26:32Z

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

TheShermanTanker · 2025-01-07T09:31:06Z

Keep open please

8342769

541d4f8

openjdk bot changed the title ~~8342769~~ 8342769: gcc port broken on HotSpot Oct 22, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 22, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 22, 2024

dholmes-ora reviewed Oct 22, 2024

View reviewed changes

TheShermanTanker changed the title ~~8342769: gcc port broken on HotSpot~~ 8342769: HotSpot Windows/gcc port is broken Oct 30, 2024

TheShermanTanker added 4 commits November 19, 2024 13:28

Revert sharedRuntime_x86.cpp

74905f0

Revert sharedRuntime.cpp

12e82f6

Merge branch 'openjdk:master' into hotspot

fb171a4

Check for _M_ARM64 instead of _WIN64 in sharedRuntime.cpp

eec28e0

TheShermanTanker added 2 commits November 20, 2024 16:45

size_t brace initialization instead of unsigned literal in test_os.cpp

e178534

size_t brace initialization instead of unsigned literals in test_os_w…

4781043

…indows.cpp

Merge branch 'openjdk:master' into hotspot

6cf1c83

swesonga approved these changes Nov 26, 2024

View reviewed changes

TheShermanTanker requested review from theRealAph and dholmes-ora December 3, 2024 07:12

theRealAph approved these changes Dec 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2024

dholmes-ora suggested changes Dec 4, 2024

View reviewed changes

TheShermanTanker added 2 commits December 4, 2024 12:23

Comment in sharedRuntimeRem.cpp

ea9190e

Typo in sharedRuntimeRem.cpp

3fe38a6

openjdk bot removed the ready Pull request is ready to be integrated label Dec 4, 2024

TheShermanTanker added 8 commits December 4, 2024 14:19

fmod_winarm64 in sharedRuntime.hpp

6a34ace

fmod_winarm64 in sharedRuntimeRem.cpp

e049e6e

fmod_winarm64 in sharedRuntime.cpp

37e0e4d

Merge branch 'openjdk:master' into hotspot

027d494

CAST_FROM_FN_PTR in os_windows.cpp

5493031

Refactor sharedRuntime.cpp

e6d8d76

AARCH64 in sharedRuntimeRem.cpp

bea150e

_WINDOWS && AARCH64 in sharedRuntime.hpp

9ecb1d3

dholmes-ora reviewed Dec 10, 2024

View reviewed changes

Merge branch 'openjdk:master' into hotspot

ff1c466

dholmes-ora approved these changes Dec 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 10, 2024

		st->print("%#11llx", reinterpret_cast<const unsigned long long>(mem_info.BaseAddress) - reinterpret_cast<const unsigned long long>(mem_info.AllocationBase));
		st->print("%#11llx", reinterpret_cast<unsigned long long>(mem_info.BaseAddress) - reinterpret_cast<unsigned long long>(mem_info.AllocationBase));

	// This code is a copy of __ieee754_fmod() formerly from the JDK's libfdlibm and is
	// used as a workaround for issues with the Windows x64 CRT implementation
	// of fmod. Microsoft has acknowledged that this is an issue in Visual Studio
	// 2012 and forward, but has not provided a time frame for a fix other than that
	// it'll not be fixed in Visual Studio 2013 or 2015.

		double SharedRuntime::fmod_winx64(double x, double y)
		double SharedRuntime::fmod_win64(double x, double y)

8342769: HotSpot Windows/gcc port is broken #21627

Are you sure you want to change the base?

8342769: HotSpot Windows/gcc port is broken #21627

Conversation

TheShermanTanker commented Oct 22, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewers without OpenJDK IDs

Reviewing

bridgekeeper bot commented Oct 22, 2024

openjdk bot commented Oct 22, 2024 • edited Loading

openjdk bot commented Oct 22, 2024

mlbridge bot commented Oct 22, 2024 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheShermanTanker commented Oct 22, 2024

TheShermanTanker commented Oct 30, 2024

dholmes-ora commented Oct 30, 2024

karianna commented Nov 2, 2024

brianjstafford commented Nov 4, 2024

TheShermanTanker commented Nov 8, 2024

swesonga commented Nov 8, 2024

TheShermanTanker commented Nov 13, 2024

brianjstafford commented Nov 13, 2024

TheShermanTanker commented Nov 20, 2024

theRealAph commented Nov 20, 2024

TheShermanTanker commented Nov 20, 2024

theRealAph commented Nov 20, 2024

theRealAph commented Nov 20, 2024

TheShermanTanker commented Nov 25, 2024

TheShermanTanker commented Dec 2, 2024

karianna commented Dec 3, 2024

TheShermanTanker commented Dec 3, 2024

theRealAph left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora commented Dec 4, 2024

TheShermanTanker commented Dec 4, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Jan 7, 2025

TheShermanTanker commented Jan 7, 2025

TheShermanTanker commented Oct 22, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 22, 2024 •

edited

Loading

mlbridge bot commented Oct 22, 2024 •

edited

Loading