Skip to content

[libc++] Fix C++23 standard modules when using with clang-cl on Windows #148992

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

siradam7th
Copy link

@siradam7th siradam7th commented Jul 15, 2025

The following PR fixes a couple of issues and compilation errors when using libcxx with clang-cl on Windows.

Issues:

  • compilation error because of missing definition of get_new_handler() in the ucrt/new.h that is included by libcxx/include/__new
  • compilation errors due to static inline in functions definition for ctime inside std.cppm.in and std.compat.cppm.in (std.cppm, std.compat.cppm).

Fixes:

  • defining get_new_handler() function under the std namespace when using _LIBCPP_ABI_VCRUNTIME.
  • defining _BUILD_STD_MODULE before including <ctime> inside std.cppm.in because MSVC's headers already have a fix in place.
  • both std.cppm and std.compat.cppm are fixed.

This pull request fixes the following issues:

@siradam7th siradam7th requested a review from a team as a code owner July 15, 2025 23:49
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 15, 2025

@llvm/pr-subscribers-libcxx

Author: Adam (siradam7th)

Changes

The following PR fixes a couple of issues and compilation errors when using libcxx with clang-cl on Windows.

Issues:

  • compilation error because of missing definition of get_new_handler() in the ucrt/new.h that is included by libcxx/include/__new
  • compilation errors due to static inline in functions definition for ctime inside std.cppm.in and std.compat.cppm.in (std.cppm, std.compat.cppm).

Fixes:

  • defining get_new_handler() function under the std namespace when using _LIBCPP_ABI_VCRUNTIME.
  • defining _BUILD_STD_MODULE before including &lt;ctime&gt; inside std.cppm.in because MSVC's headers already have a fix in place.
  • both std.cppm and std.compat.cppm are fixed.

This pull request fixes the following issues:


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

4 Files Affected:

  • (modified) libcxx/include/__new/new_handler.h (+5)
  • (modified) libcxx/modules/std.compat/ctime.inc (+4-2)
  • (modified) libcxx/modules/std.cppm.in (+3)
  • (modified) libcxx/modules/std/ctime.inc (+4-2)
diff --git a/libcxx/include/__new/new_handler.h b/libcxx/include/__new/new_handler.h
index 05f4e846c3ef9..2350f9ae9bc19 100644
--- a/libcxx/include/__new/new_handler.h
+++ b/libcxx/include/__new/new_handler.h
@@ -17,6 +17,11 @@
 
 #if defined(_LIBCPP_ABI_VCRUNTIME)
 #  include <new.h>
+// <new.h> does not define 'get_new_handler'
+// which makes the 'std' module build fail, this fixes it
+namespace std {
+_LIBCPP_EXPORTED_FROM_ABI new_handler get_new_handler() _NOEXCEPT;
+}
 #else
 _LIBCPP_BEGIN_UNVERSIONED_NAMESPACE_STD
 typedef void (*new_handler)();
diff --git a/libcxx/modules/std.compat/ctime.inc b/libcxx/modules/std.compat/ctime.inc
index eba8234a08969..7547b055b6807 100644
--- a/libcxx/modules/std.compat/ctime.inc
+++ b/libcxx/modules/std.compat/ctime.inc
@@ -14,15 +14,17 @@ export {
 
   using ::timespec _LIBCPP_USING_IF_EXISTS;
   using ::tm _LIBCPP_USING_IF_EXISTS;
-
   using ::asctime _LIBCPP_USING_IF_EXISTS;
   using ::clock _LIBCPP_USING_IF_EXISTS;
+  using ::strftime _LIBCPP_USING_IF_EXISTS;
+
+#ifndef _LIBCPP_ABI_VCRUNTIME
   using ::ctime _LIBCPP_USING_IF_EXISTS;
   using ::difftime _LIBCPP_USING_IF_EXISTS;
   using ::gmtime _LIBCPP_USING_IF_EXISTS;
   using ::localtime _LIBCPP_USING_IF_EXISTS;
   using ::mktime _LIBCPP_USING_IF_EXISTS;
-  using ::strftime _LIBCPP_USING_IF_EXISTS;
   using ::time _LIBCPP_USING_IF_EXISTS;
   using ::timespec_get _LIBCPP_USING_IF_EXISTS;
+#endif
 } // export
diff --git a/libcxx/modules/std.cppm.in b/libcxx/modules/std.cppm.in
index 984b18321923c..f19dc07169fda 100644
--- a/libcxx/modules/std.cppm.in
+++ b/libcxx/modules/std.cppm.in
@@ -51,6 +51,9 @@ module;
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
+#ifdef _LIBCPP_ABI_VCRUNTIME
+#define _BUILD_STD_MODULE
+#endif
 #include <ctime>
 #include <cuchar>
 #include <cwchar>
diff --git a/libcxx/modules/std/ctime.inc b/libcxx/modules/std/ctime.inc
index 5bfa61917e5f2..e9f8ecb6ec63e 100644
--- a/libcxx/modules/std/ctime.inc
+++ b/libcxx/modules/std/ctime.inc
@@ -14,15 +14,17 @@ export namespace std {
 
   using std::timespec _LIBCPP_USING_IF_EXISTS;
   using std::tm _LIBCPP_USING_IF_EXISTS;
-
   using std::asctime _LIBCPP_USING_IF_EXISTS;
   using std::clock _LIBCPP_USING_IF_EXISTS;
+  using std::strftime _LIBCPP_USING_IF_EXISTS;
+
+#ifndef _LIBCPP_ABI_VCRUNTIME
   using std::ctime _LIBCPP_USING_IF_EXISTS;
   using std::difftime _LIBCPP_USING_IF_EXISTS;
   using std::gmtime _LIBCPP_USING_IF_EXISTS;
   using std::localtime _LIBCPP_USING_IF_EXISTS;
   using std::mktime _LIBCPP_USING_IF_EXISTS;
-  using std::strftime _LIBCPP_USING_IF_EXISTS;
   using std::time _LIBCPP_USING_IF_EXISTS;
   using std::timespec_get _LIBCPP_USING_IF_EXISTS;
+#endif
 } // namespace std

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

@siradam7th
Copy link
Author

I have moved the macro definition to the file generate_libcxx_cppm_in.py.
I also fixed an encoding problem with that same file that stops it from running on Windows.

using ::asctime _LIBCPP_USING_IF_EXISTS;
using ::clock _LIBCPP_USING_IF_EXISTS;
using ::strftime _LIBCPP_USING_IF_EXISTS;

#ifndef _LIBCPP_ABI_VCRUNTIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you simply leaving these functions missing when using UCRT?

Also, it seem that we should modify this line libcxx/utils/libcxx/test/features.py to enable the tests:

or ("_WIN32" in compilerMacros(cfg) and not _mingwSupportsModules(cfg))

Copy link
Author

Choose a reason for hiding this comment

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

I'm looking into it, however it seems that moving the definition '_BUILD_STD_MODULE' triggered some other weird errors, I'll do more testing tomorrow and see...

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, these lines should be removed. Now get_new_handler.pass.cpp should always pass.

// FIXME: When libc++ is linked against vcruntime (i.e. the default config in
// MSVC mode), the declarations of std::set_new_handler and std::get_new_handler
// are provided by vcruntime/UCRT's new.h. However, that header actually only
// declares set_new_handler - it's missing a declaration of get_new_handler.
// XFAIL: msvc && stdlib=libc++

Copy link
Author

Choose a reason for hiding this comment

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

It looks like just defining the macro is not enough, I have resorted to defining the actual functions as inline, I'll push the changes once I made sure that everything works with both std and std.compact.

Copy link
Member

Choose a reason for hiding this comment

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

The _LIBCPP_USING_IF_EXISTS attribute is intended to make things work when the declarations are not available, and avoid precisely these #ifdef blocks. What's the issue here?

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Jul 19, 2025

Choose a reason for hiding this comment

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

The issue on these <ctime> functions is that UCRT incorrectly adds static to them, so we can't directly export them. At this time we might simply skip export for them to make the standard modules buildable. MSVC STL chooses to reimplement them in the std namespace as a workaround.

Copy link
Author

Choose a reason for hiding this comment

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

Also, it seem that we should modify this line libcxx/utils/libcxx/test/features.py to enable the tests:

or ("_WIN32" in compilerMacros(cfg) and not _mingwSupportsModules(cfg))

Is just removing that line enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can try this:

or ("_WIN32" in compilerMacros(cfg) and "__MINGW32__" in compilerMacros(cfg) and not _mingwSupportsModules(cfg))

Copy link
Author

Choose a reason for hiding this comment

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

I'm not familiar with how the tests are run/generated yet, I'll see...

Copy link

github-actions bot commented Jul 16, 2025

✅ With the latest revision this PR passed the Python code formatter.

@@ -17,6 +17,11 @@

#if defined(_LIBCPP_ABI_VCRUNTIME)
# include <new.h>
Copy link
Member

Choose a reason for hiding this comment

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

How does the MSVC STL get their own definition for get_new_handler()? It seems really brittle to include <new.h> but have our definition for get_new_handler(). We should either declare all of them (set_new_handler(), the new_handler alias, etc) or rely on <new.h> to provide them, but not a mixture of both.

Copy link
Contributor

Choose a reason for hiding this comment

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

MSVC STL just (additionally) declares it in <new>. The declaration is not available from <new.h> or <vcruntime_new.h>.

Copy link
Contributor

Choose a reason for hiding this comment

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

@siradam7th I think it would be better to follow what MSVC STL does. We can just avoid special-casing _LIBCPP_ABI_VCRUNTIME, and consistently declare these 3 things ourselves.

Copy link
Author

Choose a reason for hiding this comment

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

@siradam7th I think it would be better to follow what MSVC STL does. We can just avoid special-casing _LIBCPP_ABI_VCRUNTIME, and consistently declare these 3 things ourselves.

I'll give it a try.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the _LIBCPP_ABI_VCRUNTIME macro conditional from libcxx/include/__new/new_handler.h.
and fixed the missing symbols which are now defined by libcxx/src/new_handler.cpp, and everything is working correctly.
apart from a couple of warnings of redeclaration since some of the cpp files include <new>:

llvm-project\build\include\c++\v1\__new/new_handler.h(20,39): warning: redeclaration of 'std::set_new_handler' should not add 'dllexport' attribute [-Wdll-attribute-on-redeclaration]
   20 | _LIBCPP_EXPORTED_FROM_ABI new_handler set_new_handler(new_handler) _NOEXCEPT;
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\ucrt\new.h(32,42): note: previous declaration is here
   32 |             _CRTIMP2 new_handler __cdecl set_new_handler(_In_opt_ new_handler _NewHandler) throw();
      |                                          ^
1 warning generated.

using ::asctime _LIBCPP_USING_IF_EXISTS;
using ::clock _LIBCPP_USING_IF_EXISTS;
using ::strftime _LIBCPP_USING_IF_EXISTS;

#ifndef _LIBCPP_ABI_VCRUNTIME
Copy link
Member

Choose a reason for hiding this comment

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

The _LIBCPP_USING_IF_EXISTS attribute is intended to make things work when the declarations are not available, and avoid precisely these #ifdef blocks. What's the issue here?

@siradam7th siradam7th force-pushed the libcxx-std-module-windows-fix branch from bec4906 to 4e7a4c9 Compare July 20, 2025 02:01
Copy link

github-actions bot commented Jul 21, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions cpp,h,inc -- libcxx/include/__new/new_handler.h libcxx/modules/std.compat/ctime.inc libcxx/modules/std/ctime.inc libcxx/src/new_handler.cpp libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp b/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp
index 2c0081703..d20a65d8b 100644
--- a/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp
@@ -8,7 +8,6 @@
 
 // test get_new_handler
 
-
 #include <new>
 #include <cassert>
 

Comment on lines +31 to +69
// A workaround for UCRT because it defines these functions
// as static and that causes the error "internal linkage cannot be exported"
#ifdef _LIBCPP_ABI_VCRUNTIME

template <int = 0>
inline char* __CRTDECL ctime(_In_ time_t const* const _Time) noexcept {
return _ctime64(_Time);
}

template <int = 0>
inline double __CRTDECL difftime(_In_ time_t const _Time1, _In_ time_t const _Time2) noexcept {
return _difftime64(_Time1, _Time2);
}

template <int = 0>
inline tm* __CRTDECL gmtime(_In_ time_t const* const _Time) noexcept {
return _gmtime64(_Time);
}

template <int = 0>
inline tm* __CRTDECL localtime(_In_ time_t const* const _Time) noexcept {
return _localtime64(_Time);
}

template <int = 0>
inline time_t __CRTDECL mktime(_Inout_ tm* const _Tm) noexcept {
return _mktime64(_Tm);
}

template <int = 0>
inline time_t __CRTDECL time(_Out_opt_ time_t* const _Time) noexcept {
return _time64(_Time);
}

template <int = 0>
inline int __CRTDECL timespec_get(_Out_ timespec* const _Ts, _In_ int const _Base) noexcept {
return _timespec64_get(reinterpret_cast<_timespec64*>(_Ts), _Base);
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like this. It makes us clearly non-conforming and working around an issue that is trivial to fix.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see how it is trivial? even MSVC's STL has the same workaround because there has been no changes in UCRT in the past years, because of those few functions modules cannot be compiled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how it is trivial?

The fix in UCRT would be very trivial. UCRT team can just remove static on these functions in less than one minute, as UCRT expects the __inline extension to behave same as inline in C++.

The crux is that nobody in the UCRT team seems to be able to work on this, and contribution from open source community is still impossible.

Copy link
Author

Choose a reason for hiding this comment

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

The crux is that nobody in the UCRT team seems to be able to work on this, and contribution from open source community is still impossible.

This is one of the main reasons for the existance of this PR, if they would've done it, they at least would have done it for MSVC's sake, but here we are...

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not a good reason to add a comparatively complex and fragile workaround, but a reason to complain to them to get their asses moving. I don't know what Microsoft's reasons are, but so far this is a very clear "we don't care" and not "we can't" to me. I have no interest in supporting anybodys "we don't care".

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, but IMO we should at least to skip export of these functions to build standard modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is definitely more palatable. I'm not sure I'd approve it; I'll have to think about that a bit more I think.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, but IMO we should at least to skip export of these functions to build standard modules.

I don't think that is a good idea, since users are discoraged from defining functions in the std namespace, and not having those function would mean missing base functionality that is expected (for libraries too), we are not talking about missing new C++ features, its libc functionality.

It's not a perfect solution, but it is what MSVC has been doing from the start, and if the change is made in UCRT (which hasn't happened for quite some time now, since ~2023), it could all be removed at that time, but that does not mean we have to wait a couple years or never to start using C++ standard modules on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that is a good idea, since users are discoraged from defining functions in the std namespace, and not having those function would mean missing base functionality that is expected (for libraries too), we are not talking about missing new C++ features, its libc functionality.

You can still #include the corresponding header as a workaround. It's not like you have to not use modules because of this.

It's not a perfect solution, but it is what MSVC has been doing from the start, and if the change is made in UCRT (which hasn't happened for quite some time now, since ~2023), it could all be removed at that time, but that does not mean we have to wait a couple years or never to start using C++ standard modules on Windows.

I don't think "look, Microsoft works around their own bugs as well" is a particularly good argument. They really can't refuse to work around trivial to fix bugs.

Comment on lines -18 to -20
#if defined(_LIBCPP_ABI_VCRUNTIME)
# include <new.h>
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we split this into a separate patch? This doesn't seem to be directly related to modules.

Copy link
Author

Choose a reason for hiding this comment

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

it is, because <new> does not define get_new_handler() and that stops standard modules from building, the change was requested by @frederick-vs-ja to simplify the previous solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also fixes other tests IIUC, so this isn't specific to modules. It just happens to surface with modules as well.

Copy link
Author

Choose a reason for hiding this comment

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

This also fixes other tests IIUC, so this isn't specific to modules. It just happens to surface with modules as well.

Without some fixes, this PR will not solve the actual problem even if it gets merged it will be broken, spliting them into different PRs would just complicate the problem even more and kick the can down the road in my opinion.

If you can list what should be broken apart from this PR I will look into it.

The only test that is currently fixed is this bf7ca14 because it is failing early on the basis of "Doesn't work on Windows" and that is no longer needed, and that seems to be on a par with what this PR tries to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, can we create another PR for get_new_handler and then proceed in this PR?

Copy link
Author

@siradam7th siradam7th Jul 23, 2025

Choose a reason for hiding this comment

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

Anyway, can we create another PR for get_new_handler and then proceed in this PR?

Sure, I'll open a new PR that has the changes of the last two commits that fix get_new_handler and reference this PR.
#150182

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Jul 23, 2025

@philnik777 @siradam7th We should determine what should be fixed in this PR. The status quo is that libc++'s std/std.compat modules are basically not buildable with UCRT, so any fix would be an improvement.

There're two related issues (#64812 and #64813).

  • libc++ currently expects UCRT's <new.h> to provide new_handler and its friends, and unconditionally using's them. This looks bad to me, because <new.h> doesn't declare get_new_handler and MSVC STL declares it in <new>.
  • lib++ unconditionally exports functions from <time.h>. However, UCRT incorrectly adds static to them, which make them not exportable. Certainly this is a fault of UCRT (see also DevCom-1126857).

We may use "incomplete" approach to just make the modules buildable, which

  • adds _LIBCPP_USING_IF_EXISTS to the export declaration of get_new_handler in new.inc, and
  • skips export declarations of <time.h> functions when using UCRT.

Or, we may make the modules more complete, which

  • correctly declares get_new_handler etc. in <__new/new_handler.h> (see also https://reviews.llvm.org/D158450), and
  • adds workaround for <time.h> functions like MSVC STL.

I think I can live with the "incomplete" approach currently, since aligned_alloc is permanently missing in UCRT and there doesn't seem any workaround. (The underlying reason is probably that pairing aligned_alloc with free is not implementable in UCRT without breaking ABI.)

@siradam7th
Copy link
Author

siradam7th commented Jul 23, 2025

I vote for the second option in order to make it more complete.

We need to at least be on par with MSVC on this issue since we are talking about Windows, and not defining these functions would just open the door for more confusion in the future to whomever uses modules, and they will create similar issues that will have the same fate as this PR anyway.

So I say we just avoid future problems given the fact that this issue has been around for quite some time now and we should deal with it once and for all.

Plus this would actually increase the adoption of C++ modules overall and allow projects to use them out of the box in a cross-platform way with the same libc++, which would encorage build systems to increase support and library developers to use them.

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.

5 participants