[crac] RFR: Win/Mac build for CRaC [v11]

[Anton Kozlov]

On Wed, 5 Jul 2023 11:22:38 GMT, Roman Marchenko <rmarchenko at openjdk.org> wrote:

>> - OS-dependent CRaC functionality moved to `crac_<os>`.* files.
>> - Data structures moved to `crac_structs.hpp`
>> - Added necessary cross-platform functionality
>> - Some local calls are put under `#ifdef` checks
>> - GHA: Enabled Crac tests run in windows build
>> - GHA: macOs build is disabled until CRaC moves to newer JDK version.
>
> Roman Marchenko has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   Disabled ContextOrderTest for win because ClaimedFD doesn't support win handles

make/modules/java.base/Launcher.gmk line 134:

> 132:   $(eval $(call SetupJdkExecutable, BUILD_PAUSEENGINE, \
> 133:       NAME := pauseengine, \
> 134:       SRC := $(TOPDIR)/src/$(MODULE)/windows/native/pauseengine, \

It looks like this line is the only reason to duplicate pauseengine build rule. Can this be changed to 

      SRC := $(TOPDIR)/src/$(MODULE)/$(OPENJDK_TARGET_OS_TYPE)/native/pauseengine, \

And unified with the Unix rule?

Same with `pauseengine` rule below.

make/modules/java.base/Launcher.gmk line 150:

> 148:       CFLAGS := $(CFLAGS_JDKEXE), \
> 149:       LDFLAGS := $(LDFLAGS_JDKEXE), \
> 150:       LIBS := advapi32.lib version.lib user32.lib, \

Does simengine require this, while pauseengine does not?

src/hotspot/cpu/x86/vm_version_x86.cpp line 753:

> 751: 
> 752: bool VM_Version::glibc_env_set(char *disable_str) {
> 753: #ifdef LINUX

All glibc* functionallity needs to be moved into linux-specific code...

src/hotspot/share/services/diagnosticCommand.cpp line 1058:

> 1056:     if (out[0] != '\0') {
> 1057:       outputStream* stream = output();
> 1058:       LINUX_ONLY(if (LinuxAttachListener::get_current_op() && LinuxAttachListener::get_current_op()->is_effectively_completed()) { stream = tty; })

A multiline comment would look better

#ifdef LINUX
...


The original code did not check LinuxAttachListener::get_current_op() == NULL, as at this point the operation should exist. I propose omit the check or replace that with assert() at most.

src/java.base/windows/native/pauseengine/pauseengine.c line 1:

> 1: /*

Are there strong reasons to duplicate the complete file? And not to introduce ifdef's to the common code? A lot of logic was duplicated with this.

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

PR Review Comment: https://git.openjdk.org/crac/pull/82#discussion_r1253370283
PR Review Comment: https://git.openjdk.org/crac/pull/82#discussion_r1253372589
PR Review Comment: https://git.openjdk.org/crac/pull/82#discussion_r1253373840
PR Review Comment: https://git.openjdk.org/crac/pull/82#discussion_r1253394284
PR Review Comment: https://git.openjdk.org/crac/pull/82#discussion_r1253398413