RFR: 8342769: HotSpot Windows/gcc port is broken [v9]

[Julian Waters]

On Mon, 9 Dec 2024 07:36:54 GMT, David Holmes <dholmes at openjdk.org> wrote:

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

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21627#discussion_r1875557388