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

[David Holmes]

On Mon, 9 Dec 2024 07:05:16 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> So why does the _M_ARM64 block  not cover all of this code instead of it appearing to be used by other platforms, when it isn't?
>
> 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.

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

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