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

Drop support for Win7 / Server 2008 R2 #4742

Merged
merged 12 commits into from
Jun 25, 2024

Conversation

StephanTLavavej
Copy link
Member

This PR for VS 2022 17.12 drops support in the STL for targeting Win7 and Server 2008 R2.

Rationale

Support for all editions of Win7 and its counterpart Server 2008 R2 will have completely ended by the time that VS 2022 17.12 ships for production use. This includes extended security updates (the final, last resort, paid program). After that point, the OSes are insecure. Security is Microsoft's top priority, so it makes no sense for the STL to attempt to support fundamentally insecure systems. Removing this machinery will improve performance for everyone running supported OSes.

I've already gotten approval from my bosses and boss-like entities. I understand that Windows 7 was popular, so some of our programmer-users might have concerns about this change. However, Windows has already made its decision, and keeping this machinery in the STL won't make Win7 systems secure. End-users need to upgrade.

End-of-support dates

The big ones are already gone:

There's one highly obscure embedded edition which is almost, but not quite, an issue:

I'm not really supposed to talk about release dates, but with VS 2022 17.11 Preview 2 released just last week, it's safe to say that the POSReady edition isn't a concern for VS 2022 17.12.

Commits

As usual, I've been careful to preserve binary compatibility.

  • Win8 WaitOnAddress is statically available.
    • No ABI impact - all removed machinery was in an unnamed namespace, or within a function.
  • Demacroize __crtWaitOnAddress => WaitOnAddress etc.
  • Preserve __std_atomic_set_api_level for bincompat, delete P1135R6_atomic_wait_vista.
    • Move enum class __std_atomic_api_level to atomic_wait.cpp, marked as preserved. It's still extern "C".
    • Drop the declaration of __std_atomic_set_api_level and mark the definition as preserved. It's still dllexported by stl/src/msvcp_atomic_wait.src.
    • Drop the unused parameter name _Requested_api_level.
    • P1135R6_atomic_wait doesn't need to verify the API level anymore.
    • Delete P1135R6_atomic_wait_vista, which existed only to test the older API level.
  • Spend fewer lines on __std_atomic_api_level by dropping the trailing comma.
  • Fuse test_atomic_wait.hpp into P1135R6_atomic_wait.
    • No changes other than dropping the duplicate banner and #pragma once.
  • Win8 __fastfail is always available.
    • No ABI impact - this is within a function.
  • Win8 GetSystemTimePreciseAsFileTime is always available.
    • <__msvc_chrono.hpp>: Update comment as system_clock is now always Precise.
    • awint.hpp: We don't need to macroize the __crt name because we'll directly call the WinAPI function. We also don't need to declare the __crt name anymore. The definition is still dllexported because it's consistently marked as _CRTIMP2.
    • winapisupp.cpp: We don't need to define and store the function pointer, which was in an unnamed namespace, so no ABI impact. Mark the __crt function as preserved for bincompat, directly call the WinAPI function.
    • xtime.cpp: Directly call the WinAPI function.
  • Increase our baseline to _STL_WIN32_WINNT_WIN8 (still overridden for the STL DLL).
  • _STL_WIN32_WINNT_WIN7 is now unused.
  • Drop comment about "If WaitOnAddress is not available".
  • Link with synchronization.lib when building the atomic_wait satellite DLL.
    • Verified that the msbuild change is necessary and sufficient.
    • For clarity, move up the #pragma comment in atomic_wait.cpp.
    • The #pragma comment is what drags in synchronization.lib for most users, but our own build (and parts of the MSVC-internal build that I'll be similarly modifying) is special because it uses /NODEFAULTLIB.
  • Cauterize usage of Win8 GetCurrentPackageId.
    • See below.

Cauterizing GetCurrentPackageId

This is technically a bonus cleanup - instead of removing machinery dealing with the conditional absence of Win8 APIs, it's removing machinery dealing with the conditional presence of Win8 APIs. I figured that it was better to review in a single PR (but an isolated commit) to understand the final state of winapisupp.cpp.

Going all the way back to VS 2015 RTM, __crtIsPackagedApp was dllexported but never declared for users. Because there are no actual clients to worry about, we can cauterize its implementation, dramatically simplifying winapisupp.cpp while preserving bincompat. The function needs to continue to exist in order to satisfy our dllexport validation, but it doesn't need to do anything useful.

Here's the programmer-archaeology. I dug up internal TFS changeset 1280651 from 2014-06-17. This changeset "Remove /guard support from toolset for Dev14 CTP2" unquestionably predated VS 2015 RTM. I downloaded $/DevDiv/pu/WinC/vctools/crt and searched for all mentions of this function:

C:\Users\stl\Downloads\crt>rg __crtIsPackagedApp
crtw32\stdcpp\filesys.h
30:     if (__crtIsPackagedApp()) \
69:     if (__crtIsPackagedApp() || AreFileApisANSI())

crtw32\misc\winapisupp.cpp
104:*__crtIsPackagedApp() - Check if the current app is a Packaged app
122:extern "C"  BOOL __cdecl __crtIsPackagedApp (

crtw32\h\awint.h
37:_CRTIMP2 BOOL __cdecl __crtIsPackagedApp(void);

The paths have changed over the years, but they're all internal to the STL's separately compiled code, not available via user-visible headers. The only usage was in our experimental filesystem implementation, so the function was unnecessarily dllexported (via _CRTIMP2). This was common back then, because we were confused about how our separately compiled code worked. In this case, different TUs within the STL's separately compiled code (whether the DLL or the static LIB) had to communicate, but they didn't have to communicate with user TUs. There is Only One STL (that DLL or static LIB) so there are no mismatch concerns. We had just mindlessly copy-pasted the _CRTIMP2 incantation.

The experimental filesystem usage was removed very early on in VS 2015's lifecycle. That was internal TFS changeset 1536084 on 2015-10-10, "Removed __crtIsPackagedApp and added FILE_FLAG_BACKUP_SEMANTICS to applicable Windows API calls."

So we can make the following changes:

  • Make __crtIsPackagedApp always return FALSE. (The value doesn't actually matter.)
    • It's already marked as preserved for bincompat, but we can drop the large comment explaining what it did.
  • Drop the two flavors of definitions for __crt_IsPackagedAppHelper. Note that it wasn't dllexported, so this has no ABI impact.
    • The first definition, for defined(_ONECORE), used WRL. Drop the machinery that we defined in an unnamed namespace (obviously no ABI impact), then drop the inclusion of <wrl/wrappers/corewrappers.h>.
    • The second definition, for !defined(_ONECORE), needed the APPMODEL_ERROR_NO_PACKAGE constant and loaded GetCurrentPackageId via a cached function pointer.
  • Drop the inclusion of <AppModel.h> (conveniently commented as providing APPMODEL_ERROR_NO_PACKAGE) and the clang-format suppression for it.
  • Drop the DEFINEFUNCTIONPOINTER, STOREFUNCTIONPOINTER for GetCurrentPackageId.
    • This function pointer is defined within an unnamed namespace, so there's definitely no ABI impact. This very slightly improves the STL's performance (one fewer function pointer to store).

No ABI impact - all removed machinery was in an unnamed namespace, or within a function.
…atomic_wait_vista`.

Move `enum class __std_atomic_api_level` to `atomic_wait.cpp`, marked as preserved. It's still `extern "C"`.

Drop the declaration of `__std_atomic_set_api_level` and mark the definition as preserved. It's still dllexported by `stl/src/msvcp_atomic_wait.src`.

Drop the unused parameter name `_Requested_api_level`.

`P1135R6_atomic_wait` doesn't need to verify the API level anymore.

Delete `P1135R6_atomic_wait_vista`, which existed only to test the older API level.
No changes other than dropping the duplicate banner and `#pragma once`.
No ABI impact - this is within a function.
`<__msvc_chrono.hpp>`: Update comment as `system_clock` is now always Precise.

awint.hpp: We don't need to macroize the `__crt` name because we'll directly call the WinAPI function.
We also don't need to declare the `__crt` name anymore. The definition is still dllexported because it's consistently marked as `_CRTIMP2`.

winapisupp.cpp: We don't need to define and store the function pointer, which was in an unnamed namespace, so no ABI impact.
Mark the `__crt` function as preserved for bincompat, directly call the WinAPI function.

xtime.cpp: Directly call the WinAPI function.
… DLL.

Verified that the msbuild change is necessary and sufficient.

For clarity, move up the `#pragma comment` in atomic_wait.cpp.
Going all the way back to VS 2015 RTM, `__crtIsPackagedApp` was dllexported but never declared for users. Because there are no actual clients to worry about, we can
cauterize its implementation, dramatically simplifying winapisupp.cpp while preserving bincompat. The function needs to continue to exist in order to satisfy our
dllexport validation, but it doesn't need to do anything useful.

Here's the programmer-archaeology. I dug up TFS changeset 1280651 from 2014-06-17.
(Internal URL: https://vstfdevdiv.corp.microsoft.com/DevDiv2/DevDiv/_versionControl/changeset/1280651/ )
This changeset "Remove /guard support from toolset for Dev14 CTP2" unquestionably predated VS 2015 RTM. I downloaded `$/DevDiv/pu/WinC/vctools/crt` and searched for
all mentions of this function:

```
C:\Users\stl\Downloads\crt>rg __crtIsPackagedApp
crtw32\stdcpp\filesys.h
30:     if (__crtIsPackagedApp()) \
69:     if (__crtIsPackagedApp() || AreFileApisANSI())

crtw32\misc\winapisupp.cpp
104:*__crtIsPackagedApp() - Check if the current app is a Packaged app
122:extern "C"  BOOL __cdecl __crtIsPackagedApp (

crtw32\h\awint.h
37:_CRTIMP2 BOOL __cdecl __crtIsPackagedApp(void);
```

The paths have changed over the years, but they're all internal to the STL's separately compiled code, not available via user-visible headers.
The only usage was in our experimental filesystem implementation, so the function was *unnecessarily* dllexported (via `_CRTIMP2`). This was common back then,
because we were confused about how our separately compiled code worked. In this case, different TUs within the STL's separately compiled code (whether the
DLL or the static LIB) had to communicate, but they didn't have to communicate with user TUs. There is Only One STL (that DLL or static LIB) so there are
no mismatch concerns. We had just mindlessly copy-pasted the `_CRTIMP2` incantation.

The experimental filesystem usage was removed very early on in VS 2015's lifecycle.
That was TFS changeset 1536084 on 2015-10-10, "Removed `__crtIsPackagedApp` and added `FILE_FLAG_BACKUP_SEMANTICS` to applicable Windows API calls."
(Internal URL: https://vstfdevdiv.corp.microsoft.com/DevDiv2/DevDiv/_versionControl/changeset/1536084/ )

So we can make the following changes:

* Make `__crtIsPackagedApp` always return `FALSE`. (The value doesn't actually matter.)
  + It's already marked as preserved for bincompat, but we can drop the large comment explaining what it did.
* Drop the two flavors of definitions for `__crt_IsPackagedAppHelper`. Note that it wasn't dllexported, so this has no ABI impact.
  + The first definition, for `defined(_ONECORE)`, used WRL. Drop the machinery that we defined in an unnamed namespace (obviously no ABI impact), then drop the inclusion of `<wrl/wrappers/corewrappers.h>`.
  + The second definition, for `!defined(_ONECORE)`, needed the `APPMODEL_ERROR_NO_PACKAGE` constant and loaded `GetCurrentPackageId` via a cached function pointer.
* Drop the inclusion of `<AppModel.h>` (conveniently commented as providing `APPMODEL_ERROR_NO_PACKAGE`) and the clang-format suppression for it.
* Drop the `DEFINEFUNCTIONPOINTER`, `STOREFUNCTIONPOINTER` for `GetCurrentPackageId`.
  + This function pointer is defined within an unnamed namespace, so there's definitely no ABI impact. This very slightly improves the STL's performance (one fewer function pointer to store) and security (one fewer function pointer available as a target).
@StephanTLavavej StephanTLavavej added the performance Must go faster label Jun 21, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 21, 2024 01:52
@@ -1927,7 +1927,6 @@ compiler option, or define _ALLOW_RTCc_IN_STL to suppress this error.
#endif // defined(MRTDLL) && !defined(_M_CEE_PURE)

#define _STL_WIN32_WINNT_VISTA 0x0600 // _WIN32_WINNT_VISTA from sdkddkver.h
Copy link
Member

Choose a reason for hiding this comment

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

Is this also unused or is it used as a "baseline"?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still test it here:

#if _STL_WIN32_WINNT < _STL_WIN32_WINNT_VISTA

In theory, this could be collapsed away, since we're either building the STL as a user, or building the separately compiled STL. I'll look into this for a followup - we can't change the DLL's export surface, but when building the static LIB we can unconditionally assume Win8+, and I don't think we're doing that right now.

@@ -10,6 +10,8 @@

#include <Windows.h>

#pragma comment(lib, "synchronization")
Copy link
Member

Choose a reason for hiding this comment

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

do we even need the pragma comment if we're linking everything manually?

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. This activates when users link the STL statically (/MT, /MTd) and drag in libcpmt[d].lib, which contains atomic_wait.obj. The #pragma comment(lib, "synchronization") then tells the linker to automatically drag in synchronization.lib.

@StephanTLavavej StephanTLavavej merged commit 0be5257 into microsoft:main Jun 25, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the win7 branch June 25, 2024 05:17
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Jul 23, 2024
This reverts commit 0be5257.

Conflict resolutions:

* yvals_core.h
  + GH 4742 dropped `_STL_WIN32_WINNT_WIN7`, then GH 4751 dropped `_STL_WIN32_WINNT_WINBLUE`.
* atomic_wait.cpp
  + GH 4742 dropped `_ATOMIC_WAIT_ON_ADDRESS_STATICALLY_AVAILABLE` machinery, then GH 4751 dropped `__std_atomic_compare_exchange_128_fallback`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants