Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8285730: unify _WIN32_WINNT settings #8428

Closed
wants to merge 4 commits into from

Conversation

MBaesken
Copy link
Member

@MBaesken MBaesken commented Apr 27, 2022

Currently we set _WIN32_WINNT at various places in the codebase; this is used to target a minimum Windows version we want to support. See also for more detailled information :
https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
Macros for Conditional Declarations
"Certain functions that depend on a particular version of Windows are declared using conditional code. This enables you to use the compiler to detect whether your application uses functions that are not supported on its target version(s) of Windows."

However currently we have quite a lot of differing settings of _WIN32_WINNT in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible would make sense because we have this setting already at java_props_md.c (so targeting older Windows versions at other places is most likely not useful).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8428/head:pull/8428
$ git checkout pull/8428

Update a local copy of the PR:
$ git checkout pull/8428
$ git pull https://git.openjdk.java.net/jdk pull/8428/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8428

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8428.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 27, 2022

👋 Welcome back mbaesken! 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 openjdk bot added the rfr Pull request is ready for review label Apr 27, 2022
@openjdk
Copy link

openjdk bot commented Apr 27, 2022

@MBaesken The following labels will be automatically applied to this pull request:

  • client
  • core-libs
  • hotspot-runtime
  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org nio nio-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 27, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 27, 2022

Webrevs

@@ -156,7 +156,7 @@ WEPOLL_EXPORT int epoll_wait(HANDLE ephnd,
#define WIN32_LEAN_AND_MEAN

#undef _WIN32_WINNT
#define _WIN32_WINNT 0x0600
#define _WIN32_WINNT 0x0601
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 3rd party code and would prefer not change it if possible.

Copy link
Member Author

@MBaesken MBaesken Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alan, I agree (thats why I did not change the define in harfbuzz, but I missed that wepoll.c is 3rd party code as well).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you can revert the update to the copyright header in the latest version as there aren't any changes to this source file now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure I did it .

@prrace
Copy link
Contributor

prrace commented Apr 27, 2022

/label build

@prrace
Copy link
Contributor

prrace commented Apr 27, 2022

This is OK by me, but I wonder if the build folks might want to think about this and whether it should be centralised somehow
since it seems odd to mandate different versions of windows in different places.

@openjdk openjdk bot added the build build-dev@openjdk.org label Apr 27, 2022
@openjdk
Copy link

openjdk bot commented Apr 27, 2022

@prrace
The build label was successfully added.

@erikj79
Copy link
Member

erikj79 commented Apr 27, 2022

Requiring different windows versions in different parts of the product doesn't make much sense, but I think it's even worse trying to keep all these different locations in sync. At least most of these have a comment explaining why that particular Windows version is required there. Changing these values invalidates those comments.

If we are to do something about this, then we need to define this macro in a central location, and verify the value in configure. Then we would provide it either on compile command lines, or in a globally included config.h.

@dholmes-ora
Copy link
Member

dholmes-ora commented Apr 27, 2022

There is obviously a lot of history here and different parts of the codebase had hit different minimum OS version requirements at different times. While at one time we would have had to cater for building on systems with and without a given API those days are long gone for the code being touched here (AFAICS). As Erik states we should be able to set a minimum _WIN32_WINNT_ value needed to build all the codebase and simply have that checked and set at configure time. The code itself would not need to define _WIN32_WINNT.

@MBaesken
Copy link
Member Author

Hi Erik/David/Phil, we already have a good central place where we set the definition of WIN32_LEAN_AND_MEAN

autoconf/flags-cflags.m4:460: ALWAYS_DEFINES_JDK="-DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE
autoconf/flags-cflags.m4:462: ALWAYS_DEFINES_JVM="-DNOMINMAX -DWIN32_LEAN_AND_MEAN"

should we add there the _WIN32_WINNT=0x0601 define (and remove the differing defines instead at the other places) ?
(defines of _WIN32_WINNT in 3rd party code would stay like wepoll.c and harfbuzz).

@erikj79
Copy link
Member

erikj79 commented Apr 28, 2022

Hi Erik/David/Phil, we already have a good central place where we set the definition of WIN32_LEAN_AND_MEAN

autoconf/flags-cflags.m4:460: ALWAYS_DEFINES_JDK="-DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_DEPRECATE autoconf/flags-cflags.m4:462: ALWAYS_DEFINES_JVM="-DNOMINMAX -DWIN32_LEAN_AND_MEAN"

should we add there the _WIN32_WINNT=0x0601 define (and remove the differing defines instead at the other places) ? (defines of _WIN32_WINNT in 3rd party code would stay like wepoll.c and harfbuzz).

Seems reasonable to me at least.

@dholmes-ora
Copy link
Member

That only seems to be half of the issue though. If we are defining _WIN32_WINNT=0x0601 because the minimum required OS API support level is Windows 7, then don't we need a check that the build platform is also at least Windows 7?

@MBaesken
Copy link
Member Author

That only seems to be half of the issue though. If we are defining _WIN32_WINNT=0x0601 because the minimum required OS API support level is Windows 7, then don't we need a check that the build platform is also at least Windows 7?

Hi David, on older OS than Windows 7 we wouldn't be able to build OJDK anyway currently. Our oldest VS we still support (see toolchain_microsoft.m4) is VS2017.
VS2017 has the following system requirements
https://docs.microsoft.com/en-us/visualstudio/releases/2017/vs2017-system-requirements-vs
see supported OS , there the oldest supported OS is Windows Server 2012 R2 and Windows 7 SP1. So on older than Win7 we would not even have a compiler anymore that passes configure.

@erikj79
Copy link
Member

erikj79 commented May 3, 2022

If we define this centrally using compiler flags, then we should not also update each location in the source. Those need to either be removed or left alone. In the cases where the source definition is guarded with an ifndef and there is a comment explaining why a particular version of windows is required, keeping it around could make sense. But on the other hand, since those defines are overridden using flags, they are likely to bit rot over time so we might as well just remove all of them.

@dholmes-ora
Copy link
Member

I agree with Erik - the source locations need to be modified to not define it. If we want to keep a record perhaps add an assertion that the value is what was expected?

I still feel we have a disconnect between this and an actual check for what the build and runtime platform version is ...

and it isn't at all clear how someone using an API only in a later Windows version, and so setting _WIN32_WINNT to a higher value, will know that this is defined down in the build files ?

@MBaesken
Copy link
Member Author

MBaesken commented May 4, 2022

Interesting fact :
we run now into this compile error :

d:\a\jdk\jdk\jdk\src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp(1262): error C2065: 'NCRYPT_CIPHER_KEY_BLOB': undeclared identifier
d:\a\jdk\jdk\jdk\src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp(1280): error C2065: 'NCRYPT_PROTECTED_KEY_BLOB': undeclared identifier

Reason seems that already for some time ( at least OpenJDK 11 (!) ) we have an implicit minimum requirement of Windows 8 / Windows 2012
APIs for this code, so enforcing Win 7 is too old :

https://docs.microsoft.com/en-us/windows/win32/api/ncrypt/nf-ncrypt-ncryptexportkey

NCRYPT_PROTECTED_KEY_BLOB
Export a protected key in a NCRYPT_KEY_BLOB_HEADER structure.

Windows 8 and Windows Server 2012: Support for this value begins.

NCRYPT_CIPHER_KEY_BLOB
Export a cipher key in a NCRYPT_KEY_BLOB_HEADER structure.

Windows 8 and Windows Server 2012: Support for this value begins.

@MBaesken
Copy link
Member Author

MBaesken commented May 4, 2022

I agree with Erik - the source locations need to be modified to not define it. If we want to keep a record perhaps add an assertion that the value is what was expected?

I still feel we have a disconnect between this and an actual check for what the build and runtime platform version is ...

and it isn't at all clear how someone using an API only in a later Windows version, and so setting _WIN32_WINNT to a higher value, will know that this is defined down in the build files ?

Hi David, I agree the other locations as long as they are not 3rd party code should be cleaned up.
Someone using an API that is only available in later Windows versions would get a redefinition warning as long as no undefine is done.
But we have to set _WIN32_WINNT anyway to a higher value (windows8, I think that's 0x0602) to compile
src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp .

@dholmes-ora
Copy link
Member

I'm confused. src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp doesn't set _WIN32_WINNT so how is that later API being enabled? Does this mean that not setting _WIN32_WINNT means :any API is allowed" ?

@MBaesken
Copy link
Member Author

MBaesken commented May 4, 2022

I'm confused. src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp doesn't set _WIN32_WINNT so how is that later API being enabled? Does this mean that not setting _WIN32_WINNT means :any API is allowed" ?

I found this info here https://stackoverflow.com/questions/37668692/visual-studio-setting-winver-win32-winnt-to-windows-8-on-windows-7
"VS 2012 uses the Windows 8.0 SDK which defaults to _WIN32_WINNT=0x0602 (Windows 8). VS 2013 / 2015 uses the Windows 8.1 SDK which defaults to _WIN32_WINNT=0x0603 (Windows 8.1). If you use VS 2015 and the Windows 10 SDK, it defaults to _WIN32_WINNT=0x0A00 (Windows 10)."

So it seems you get some more or less recent default when working with a not too old compiler/SDK.

@MBaesken
Copy link
Member Author

MBaesken commented May 6, 2022

Does this mean that not setting _WIN32_WINNT means :any API is allowed" ?

Hi David , I did one more try with my current setup (VS2017 on a Win10 notebook). I did not set _WIN32_WINNT.
My little test program

#include <windows.h>
#include <stdio.h>

int main() {

#ifdef _WIN32_WINNT
  printf("_WIN32_WINNT is defined .\n");

#if (_WIN32_WINNT == 0x0600)
  printf("Vista API setting\n");
#endif

#if (_WIN32_WINNT == 0x0601)
  printf("Win 7 API setting\n");
#endif

#if (_WIN32_WINNT == 0x0602)
  printf("Win 8 API setting\n");
#endif

#if (_WIN32_WINNT == 0x0603)
  printf("Win 8.1 API setting\n");
#endif

#if (_WIN32_WINNT == 0x0A00)
  printf("Win 10 API setting\n");
#endif

#endif

  return 0;
}

shows me
_WIN32_WINNT is defined .
Win 10 API setting

So I think with our current compilers in use like VS2017 / VS2019 we allow Win10 APIs in most of our code except a few places where we set _WIN32_WINNT and go back to some mixture of older APIs.
Not sure if this is a good thing, we could break for example Win 8.1/Win2012 compatibility easily this way.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems okay to me in this form.

I agree that explicitly setting this is better than unknowingly using API's that might not be available on supported platforms.

Thanks.

@openjdk
Copy link

openjdk bot commented May 6, 2022

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

8285730: unify _WIN32_WINNT settings

Reviewed-by: dholmes, erikj, ihse, prr, alanb

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:

  • 9a3cb93: 8030121: java/awt/dnd/MissingDragExitEventTest/MissingDragExitEventTest.java fails
  • ace4230: 8261650: Add a comment with details for MTLVC_MAX_INDEX
  • 2939553: 8282351: jpackage does not work if class file has $$ in the name on windows
  • 61450bb: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
  • c28a636: 8286442: ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 54e3308: 8286348: incorrect use of @serial
  • b0d2b0a: 8286331: jni_GetStringUTFChars() uses wrong heap allocator
  • 902b1dd: 8284686: Interval of < 1 ms disables ExecutionSample events
  • 02e5fc0: 8286435: JDK-8284316 caused validate-source to fail in Tier1
  • 6a7c023: 8284316: Support accessibility ManualTestFrame.java for non SwingSet tests
  • ... and 163 more: https://git.openjdk.java.net/jdk/compare/4919525ddb55ba52d199a37c3b0e14e4a0c7c738...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 openjdk bot added the ready Pull request is ready to be integrated label May 6, 2022
@MBaesken
Copy link
Member Author

MBaesken commented May 6, 2022

Hi David, thanks for the review !

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for doing this cleanup.

@MBaesken
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 10, 2022

Going to push as commit 4fd79a6.
Since your change was applied there have been 174 commits pushed to the master branch:

  • bd6026c: 7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell.
  • 9a3cb93: 8030121: java/awt/dnd/MissingDragExitEventTest/MissingDragExitEventTest.java fails
  • ace4230: 8261650: Add a comment with details for MTLVC_MAX_INDEX
  • 2939553: 8282351: jpackage does not work if class file has $$ in the name on windows
  • 61450bb: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
  • c28a636: 8286442: ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 54e3308: 8286348: incorrect use of @serial
  • b0d2b0a: 8286331: jni_GetStringUTFChars() uses wrong heap allocator
  • 902b1dd: 8284686: Interval of < 1 ms disables ExecutionSample events
  • 02e5fc0: 8286435: JDK-8284316 caused validate-source to fail in Tier1
  • ... and 164 more: https://git.openjdk.java.net/jdk/compare/4919525ddb55ba52d199a37c3b0e14e4a0c7c738...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 10, 2022
@openjdk openjdk bot closed this May 10, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 10, 2022
@openjdk
Copy link

openjdk bot commented May 10, 2022

@MBaesken Pushed as commit 4fd79a6.

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

@mkartashev
Copy link
Member

This change seem to have made this group of declarations obsolete:
src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157 (follow the link). Does anyone have plans to drop those? Have any bugs been filed for that? If not, I'll attend to that myself.

@dholmes-ora
Copy link
Member

@mkartashev all references to WINVER in the AWT code seem obsolete. Possibly most of the IS_WINxxx usages could also be replaced.

@MBaesken
Copy link
Member Author

This change seem to have made this group of declarations obsolete: src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157 (follow the link). Does anyone have plans to drop those? Have any bugs been filed for that? If not, I'll attend to that myself.

Hi Maxim, no plans from me, so feel free to do further cleanups.

@mkartashev
Copy link
Member

@dholmes-ora @MBaesken Thank you! Filed JDK-8286634.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated nio nio-dev@openjdk.org
7 participants