-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[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
base: main
Are you sure you want to change the base?
Changes from 5 commits
ed2c4e4
b6bf097
4e7a4c9
bf7ca14
b64ac1e
78a0e71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,15 +14,57 @@ 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 | ||||||||||||||||||||
|
or ("_WIN32" in compilerMacros(cfg) and not _mingwSupportsModules(cfg)) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Lines 11 to 17 in 653872f
// 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++ | |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, itslibc
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.
Uh oh!
There was an error while loading. Please reload this page.