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

Toolset update: VS 2022 17.3 Preview 1 #2714

Merged
merged 9 commits into from
May 12, 2022

Conversation

StephanTLavavej
Copy link
Member

  • This updates the toolset to VS 2022 17.3 Preview 1 (and the May 2022 Patch Tuesday). Effectively the only changes are compiler bugfixes since Toolset update: VS 2022 17.2 Preview 3 #2651 updated to 17.2 Preview 3.
    • Also use PowerShell 7.2.3 to prepare the VMSS.
  • In stl/CMakeLists.txt, we don't need to include crt/src/concrt anymore.
  • Mention the Windows 11 SDK in the README.
    • The MSVC-internal build already uses the 22000 SDK.
    • Taking this dependency (for building/testing the STL only; consumers of the STL are unaffected as we don't drag in <Windows.h> from headers) allows us to remove a couple of long-standing workarounds.
  • In filesystem.cpp, we can directly use FILE_DISPOSITION_INFO_EX now.
    • I verified with static_assert that these structs are layout-compatible, and that the constants have identical values.
    • In CMakeLists.txt, we need to increase NTDDI_VERSION in order to get the declaration of FileDispositionInfoEx. Instead of increasing it to the exact value we need, I am going further and increasing it to the maximum supported value right now, NTDDI_WIN10_CO, so that we can access other declarations if necessary (see the comment above).
    • Technically we don't need to increase _WIN32_WINNT (due to an apparent mistake in the WinSDK headers, the FILE_DISPOSITION_FLAG_DELETE etc. macros are effectively unguarded), but I am increasing it to its maximum value of Win10 for consistency.
    • Will be paired with MSVC-internal changes to src/vctools/crt/crt-common.settings.targets, setting Win32WinNt and NtddiVersion.
  • Fix Tests: stop disabling warning 5105: "macro expansion producing 'defined' has undefined behavior" #854 by removing the winsdk matrix workaround. This simply deletes all of the winsdk matrices (which were burdensome to maintain, and had accumulated a small amount of unintentional divergence) and changes the affected tests to use the corresponding plain matrices.
    • ⚠️ This is inherently a stealth build break for PRs in flight - we will need to watch out for any that are adding tests with the winsdk matrix.
  • Remove /Za test coverage. (No product changes.)
    • This deletes all of the /Za configurations from the remaining matrices (we never combined them with unique options so they can simply be dropped) and removes some test workarounds.
    • This is needed if we want to drop the winsdk matrix workarounds (as the WinSDK is still incompatible with /Za).
    • /Za has always been problematically buggy, but it also used to be the only way to request extra conformance from MSVC. Now, /permissive- and /Zc:preprocessor have morally superseded /Za, and /Za is officially discouraged in the documentation. We also have excellent conformance coverage from Clang in its strict mode and EDG, so it has been aeons since /Za found anything in the STL that needed to be changed. Removing this test coverage doesn't block users from using /Za with the STL, and we'll continue to fix any reported conformance issues in the library, but we won't spend effort on working around /Za bugs in the product and test code.
  • Fix a single-underscore _declspec in the ancient Dev09_056375_locale_cleanup/testdll.cpp.
    • This is very distantly related to the toolset update (after removing the winsdk matrix workaround but before removing /Za, the compiler emitted errors in the WinSDK headers followed by complaining about this single-underscore extension; we should fix it even though we're dropping /Za coverage).

@StephanTLavavej StephanTLavavej added infrastructure Related to repository automation test Related to test code filesystem C++17 filesystem labels May 11, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner May 11, 2022 23:27
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This LGTM, but I don't feel comfortable enough with the codebase yet to be a final reviewer.

@StephanTLavavej
Copy link
Member Author

Mirrored to internal MSVC-PR-398667 with passing tests. (I can easily push changes to both for code review feedback.)

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Verified there are no remaining mentions of /Za and winsdk, and no files with winsdk in their names.

CMakeLists.txt Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem C++17 filesystem infrastructure Related to repository automation test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests: stop disabling warning 5105: "macro expansion producing 'defined' has undefined behavior"
3 participants