-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[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 all commits
ed2c4e4
b6bf097
4e7a4c9
bf7ca14
b64ac1e
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 | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Aha, these lines should be removed. Now Lines 11 to 17 in 653872f
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue on these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is just removing that line enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can try this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
||||||||||||||||||||
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 | ||||||||||||||||||||
|
||||||||||||||||||||
// 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 | ||||||||||||||||||||
Comment on lines
+31
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
The fix in UCRT would be very trivial. UCRT team can just remove 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, but IMO we should at least to skip There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 It's not a perfect solution, but it is what MSVC has been doing from the start, and if the change is made in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You can still
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. |
||||||||||||||||||||
} // export |
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.
Can we split this into a separate patch? This doesn't seem to be directly related to 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.
it is, because
<new>
does not defineget_new_handler()
and that stops standard modules from building, the change was requested by @frederick-vs-ja to simplify the previous solution.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.
This also fixes other tests IIUC, so this isn't specific to modules. It just happens to surface with modules as well.
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.
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.
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.
Anyway, can we create another PR for
get_new_handler
and then proceed in this PR?Uh oh!
There was an error while loading. Please reload this page.
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.
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