-
Notifications
You must be signed in to change notification settings - Fork 232
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
Make compatible with GCC #1245
Make compatible with GCC #1245
Conversation
GNU assembler treats symbols starting with uppercase `L` as local symbols and discard them, unless they have been defined as global.
When calling co_await with the return value of resume_after and resume_on_signal, GCC seems to require a move. This may be an upstream bug, but work around it for now by providing the move constructor for GCC. This might be related to upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99575
Make thread_pool copyable by making m_pool a std::shared_ptr, but only for GCC. This might be related to upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103963
The expansion of the REQUIRE macro ends up causing the co_await expression inside to be run twice when compiled with GCC, which causes the test to fail. Now this may be a compiler bug, but the workaround is simple.
libstdc++ uses nanoseconds for system_clock, which causes signed overflow when offset by the epoch inside clock::from_sys. The fix is to reduce its resolution before passing it to clock::from_sys, instead of after.
...so that it won't rebuild cppwinrt every single time.
The test doesn't build due to meomory limitation.
libstdc++ uses nanoseconds for system_clock, which causes signed overflow when offset by the epoch inside clock::to_sys. The fix is to do time_point_ca *after* passing it to clock::to_sys, instead of before.
This is not available in older mingw-w64 headers.
@@ -272,7 +272,6 @@ TEST_CASE("hstring,operator,std::wstring_view") | |||
REQUIRE(L"abc" == ws); | |||
|
|||
hs.clear(); | |||
REQUIRE(L"abc" == ws); |
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 sure what's the point of this test because this seems to be a use-after-free, and it crashes when run with Application Verifier enabled. So I removed it.
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.
Good catch.
Ah the msys2 workflow uses the |
strings/base_includes.h
Outdated
@@ -27,8 +27,10 @@ | |||
|
|||
#if __has_include(<windowsnumerics.impl.h>) | |||
#define WINRT_IMPL_NUMERICS | |||
#if __has_include(<directxmath.h>) |
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.
Is it intentional that directxmath.h is only included if windowsnumeric.impl.h is available?
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 was done to work around a C++ modules bug. It's required by numerics but not by cppwinrt itself.
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.
Feel free to remove it if it helps.
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.
Ah, can probably keep for now and I'll keep it in mind when I look into modules again soon
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 realized that this change is not actually needed to fix anything so I will undo it and leave this file unchanged. (The error I got is from mixing old headers with the downloaded windowsnumerics.impl.h
for the tests, so not an issue that will affect real users.)
This reverts commit 0129567.
done |
This reverts commit e87b249.
GCC does not compile co_await on thread_pool because it wants a copy constructor. Disabling this test for now. This might be related to upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103963
FYI - I don't see any problems with these changes besides what you already addressed. The details are beyond my expertise so I'm not going to officially sign off. (I am not a contributor so my signoff wouldn't help much anyway). |
// on the return value of resume_on_signal. | ||
// This might be related to upstream bug: | ||
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99575 | ||
awaitable(awaitable &&other) noexcept : |
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't we use = default
to force the compiler to generate this?
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.
IIRC the issue is that std::atomic<T>
is unmovable (copy constructor is deleted).
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.
Ah, that's too bad.
Changes for GCC compatibility:
winrt::impl::bind_in
mentioned in Partial GCC compatibility improvements #1234 (upstream: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61663)WINRT_IMPL_LINK
macro to properly declare the symbol as global (GNUC).resume_after
andresume_on_signal
because GCC adds useless moves inco_await
(suspect to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99575)Added GCC-only hack copy constructor tothread_pool
because GCC seems to want it when building thethread_pool.cpp
test (suspect to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103963)Note that this also changes them_pool
field to be astd::shared_ptr
.winrt::clock
ifsystem_clock
has nanosecond resolutionGeneral changes:
Include(reverted)directxmath.h
only if it exists (for compatibility with older mingw-w64 headers)I am not happy with the move/copy constructor hacks, but they seem unavoidable unless GCC have their bugs fixed...
CC @Biswa96 @mstorsjo