Skip to content

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Dec 4, 2023

Thanks to @mstorsjo's observation in #74182 (comment), here's a far less intrusive way to make libc++'s filesystem tests compatible with MSVC's STL.

In msvc_stdlib_force_include.h (which, as the filename indicates, is force-included by the MSVC STL test environment, but never included by the libc++ test environment), we need to define 3 more macros:

  • _CRT_DECLARE_NONSTDC_NAMES activates the POSIX names of getcwd etc. As the comment explains, we need this because we test with Clang -fno-ms-compatibility, which defines __STDC__ to 1, which causes the UCRT headers to disable the POSIX names by default.
  • Then we need _CRT_NONSTDC_NO_WARNINGS to avoid emitting deprecation warnings about the POSIX names.
  • Finally, we need NOMINMAX to seal away the ancient evil.

Like the other macros, these are defined within #ifndef _LIBCXX_IN_DEVCRT because we want them only within libc++'s test suite. (Our own test suite, formerly called "devcrt", reaches into our llvm-project submodule to include this header in a few places, and we don't want these macros defined there.)

While I'm in the neighborhood, I'm also adding a "simulated" macro: __has_feature(hwaddress_sanitizer) as 0.

I pushed a commit to clang-format all of msvc_stdlib_force_include.h. (The CI's suggested diff apparently clang-formatted just the modified lines, which led to an inconsistently gross look, so I figured it was time to format the whole thing.)

Finally, as @philnik777 requested, I'm removing __has_builtin(__builtin_source_location) guards, including in libc++ product code. <version>, source_location.version.compile.pass.cpp (which now passes for MSVC's STL), and version.version.compile.pass.cpp were regenerated, no manual changes.

@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 4, 2023 01:03
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 4, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2023

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Thanks to @mstorsjo's observation in #74182 (comment), here's a far less intrusive way to make libc++'s filesystem tests compatible with MSVC's STL.

In msvc_stdlib_force_include.h (which, as the filename indicates, is force-included by the MSVC STL test environment, but never included by the libc++ test environment), we need to define 3 more macros:

  • _CRT_DECLARE_NONSTDC_NAMES activates the POSIX names of getcwd etc. As the comment explains, we need this because we test with Clang -fno-ms-compatibility, which defines __STDC__ to 1, which causes the UCRT headers to disable the POSIX names by default.
  • Then we need _CRT_NONSTDC_NO_WARNINGS to avoid emitting deprecation warnings about the POSIX names.
  • Finally, we need NOMINMAX to seal away the ancient evil.

Like the other macros, these are defined within #ifndef _LIBCXX_IN_DEVCRT because we want them only within libc++'s test suite. (Our own test suite, formerly called "devcrt", reaches into our llvm-project submodule to include this header in a few places, and we don't want these macros defined there.)

While I'm in the neighborhood, I'm also updating a couple of "simulated" macros. This simulates __has_feature(hwaddress_sanitizer) as 0, and __has_builtin(__builtin_source_location) as 1. The latter allows the source_location feature-test macro test to pass.


Full diff: https://github.com/llvm/llvm-project/pull/74266.diff

1 Files Affected:

  • (modified) libcxx/test/support/msvc_stdlib_force_include.h (+18-5)
diff --git a/libcxx/test/support/msvc_stdlib_force_include.h b/libcxx/test/support/msvc_stdlib_force_include.h
index 3c61f0b880b18..1f5c1269ee00e 100644
--- a/libcxx/test/support/msvc_stdlib_force_include.h
+++ b/libcxx/test/support/msvc_stdlib_force_include.h
@@ -18,6 +18,15 @@
 
     // Avoid assertion dialogs.
     #define _CRT_SECURE_INVALID_PARAMETER(EXPR) ::abort()
+
+    // Declare POSIX function names. (By default, Clang -fno-ms-compatibility causes them to be omitted.)
+    #define _CRT_DECLARE_NONSTDC_NAMES 1
+
+    // Silence warnings about POSIX function names.
+    #define _CRT_NONSTDC_NO_WARNINGS 1
+
+    // Avoid Windows.h macroizing min() and max().
+    #define NOMINMAX 1
 #endif // _LIBCXX_IN_DEVCRT
 
 #include <crtdbg.h>
@@ -45,15 +54,19 @@ const AssertionDialogAvoider assertion_dialog_avoider{};
 #if !defined(__clang__)
     // Simulate feature-test macros.
     #define __has_feature(X) _MSVC_HAS_FEATURE_ ## X
-    #define _MSVC_HAS_FEATURE_cxx_exceptions    1
-    #define _MSVC_HAS_FEATURE_cxx_rtti          1
-    #define _MSVC_HAS_FEATURE_address_sanitizer 0
-    #define _MSVC_HAS_FEATURE_memory_sanitizer  0
-    #define _MSVC_HAS_FEATURE_thread_sanitizer  0
+    #define _MSVC_HAS_FEATURE_cxx_exceptions      1
+    #define _MSVC_HAS_FEATURE_cxx_rtti            1
+    #define _MSVC_HAS_FEATURE_address_sanitizer   0
+    #define _MSVC_HAS_FEATURE_hwaddress_sanitizer 0
+    #define _MSVC_HAS_FEATURE_memory_sanitizer    0
+    #define _MSVC_HAS_FEATURE_thread_sanitizer    0
 
     #define __has_attribute(X) _MSVC_HAS_ATTRIBUTE_ ## X
     #define _MSVC_HAS_ATTRIBUTE_vector_size     0
 
+    #define __has_builtin(X) _MSVC_HAS_BUILTIN_ ## X
+    #define _MSVC_HAS_BUILTIN___builtin_source_location 1
+
     // Silence compiler warnings.
     #pragma warning(disable: 4180) // qualifier applied to function type has no meaning; ignored
     #pragma warning(disable: 4324) // structure was padded due to alignment specifier

@github-actions
Copy link

github-actions bot commented Dec 4, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

<version>, source_location.version.compile.pass.cpp, and version.version.compile.pass.cpp were regenerated, no manual changes.
@StephanTLavavej
Copy link
Member Author

@philnik777 Looks like removing __has_builtin(__builtin_source_location) worked! I updated the PR description accordingly.

@StephanTLavavej StephanTLavavej changed the title [libc++][test] Update msvc_stdlib_force_include.h [libc++] Update <source_location> and msvc_stdlib_force_include.h Dec 4, 2023
@ldionne ldionne merged commit 3aee4a9 into llvm:main Dec 6, 2023
@StephanTLavavej StephanTLavavej deleted the stl-18-power-word-posix branch December 6, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants