Make compatible with GCC by alvinhochun · Pull Request #1245 · microsoft/cppwinrt

alvinhochun · Dec 11, 2022

Changes for GCC compatibility:

Added GCC-only hack for winrt::impl::bind_in mentioned in Partial GCC compatibility improvements #1234 (upstream: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61663)
Fixed the WINRT_IMPL_LINK macro to properly declare the symbol as global (GNUC).
Added GCC-only hack move constructors to the awaitables in resume_after and resume_on_signal because GCC adds useless moves in co_await (suspect to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99575)
~~Added GCC-only hack copy constructor to thread_pool because GCC seems to want it when building the thread_pool.cpp test (suspect to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103963)~~
- ~~Note that this also changes the m_pool field to be a std::shared_ptr.~~
- (reverted)
Fixed signed overflow in winrt::clock if system_clock has nanosecond resolution
Miscellaneous test fixes and disabled some unbuildable or unreliable tests
Added CI for GCC using msys2
- I also tried adding a cross build test, but Ubuntu has only GCC 10 and even the mingw-w64 on Arch Linux isn't recent enough to have sufficiently working headers (some fixes are not in the latest release).

General changes:

Enabled colour diagnostics and test output for all CI builds
~~Include directxmath.h only if it exists (for compatibility with older mingw-w64 headers)~~ (reverted)
Tagged async_propagate_cancel test as mayfail so that it doesn't block CI (please fix with Cancel propagation broken for nested IAsyncAction after #1228 #1243)

I am not happy with the move/copy constructor hacks, but they seem unavoidable unless GCC have their bugs fixed...

Upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61663

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.

Ref: microsoft#1243

alvinhochun · Dec 11, 2022

test/old_tests/UnitTests/hstring.cpp

    REQUIRE(L"abc" == ws);

    hs.clear();
-    REQUIRE(L"abc" == ws);


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.

Good catch.

alvinhochun · Dec 11, 2022

Ah the msys2 workflow uses the msys2/setup-msys2@v2 action. @kennykerr can you allow it too?

sylveon · Dec 11, 2022

strings/base_includes.h


 #if __has_include(<windowsnumerics.impl.h>)
 #define WINRT_IMPL_NUMERICS
+#if __has_include(<directxmath.h>)


Is it intentional that directxmath.h is only included if windowsnumeric.impl.h is available?

This was done to work around a C++ modules bug. It's required by numerics but not by cppwinrt itself.

Feel free to remove it if it helps.

Ah, can probably keep for now and I'll keep it in mind when I look into modules again soon

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.

strings/base_coroutine_threadpool.h

kennykerr · Dec 12, 2022

Ah the msys2 workflow uses the msys2/setup-msys2@v2 action. @kennykerr can you allow it too?

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

test/test/custom_error.cpp

dmachaj · Dec 12, 2022

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).

kennykerr · Dec 13, 2022

strings/base_coroutine_threadpool.h

+            // 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 :


Can't we use = default to force the compiler to generate this?

IIRC the issue is that std::atomic<T> is unmovable (copy constructor is deleted).

Ah, that's too bad.

alvinhochun added 26 commits December 9, 2022 00:02

Hack around GCC issue with using winrt::impl::bind_in.

a9f0f8d

Upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61663

Fix WINRT_IMPL_LINK weak symbol being discarded on GCC

87d2dd0

GNU assembler treats symbols starting with uppercase `L` as local symbols and discard them, unless they have been defined as global.

Work around GCC wanting copy constructor in thread_pool

e87b249

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

cmake: Stop adding Clang-specific flag when using GCC

1406d1f

Fix custom_error.cpp tests on GCC

f323371

Disable code in weak.cpp test that doesn't build with GCC

089ae3e

Fix async, resume_on_signal test

47222ab

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.

GCC: Fix clock::from_time_t with libstdc++

d6b56d6

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.

cmake: Remove the update step for winmd

36bb18a

...so that it won't rebuild cppwinrt every single time.

GCC: Work around out_params_bad.cpp miscompilation

f3f838d

GCC: Work around multi_threaded_vector.cpp miscompilation

f90f754

GCC: Disable test_old/agile_ref that randomly crashes

1f4edae

Remove use-after-free in hstring test

2b37b0a

CI: Add GCC tests using MSYS2

5a5fc09

CI/cmake: Enable colour test output

8cdacaa

CI: Enable CMake colour diagnostics

3ea3663

CI: Update msys2 packages

2ad00ef

CI/msys2: Add mingw64 build

bb78632

CI: Skip building large PCH for i686

9c9627b

CI: Remove msys2 i686 Debug build

e7d0d09

The test doesn't build due to meomory limitation.

GCC: Fix clock::to_time_t with libstdc++

0f0a3ef

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.

Include directxmath.h only if it exists

0129567

This is not available in older mingw-w64 headers.

CI: Add cross-built tests with GCC

24cd40f

CI: Disable GCC cross build (mingw-w64 headers are still too old)

ed9d296

test: Tag async_propagate_cancel test as mayfail

e2cd4df

Ref: microsoft#1243

alvinhochun commented Dec 11, 2022

View reviewed changes

sylveon reviewed Dec 11, 2022

View reviewed changes

Revert "Include directxmath.h only if it exists"

9645cf0

This reverts commit 0129567.

kennykerr reviewed Dec 12, 2022

View reviewed changes

strings/base_coroutine_threadpool.h Outdated Show resolved Hide resolved

alvinhochun added 2 commits December 13, 2022 00:40

Revert "Work around GCC wanting copy constructor in thread_pool"

cc37b3f

This reverts commit e87b249.

test: Disable thread_pool.cpp tests for GCC

9d4583f

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

dmachaj reviewed Dec 12, 2022

View reviewed changes

test/test/custom_error.cpp Outdated Show resolved Hide resolved

test: Remove redundant checks in test/custom_error.cpp

75d5c6e

dmachaj reviewed Dec 12, 2022

View reviewed changes

test/test/custom_error.cpp Outdated Show resolved Hide resolved

Fix typo

fb9d105

kennykerr reviewed Dec 13, 2022

View reviewed changes

kennykerr approved these changes Dec 13, 2022

View reviewed changes

kennykerr merged commit 3343c7c into microsoft:master Dec 13, 2022

sylveon mentioned this pull request Mar 11, 2023

Windows 11 context menu notepad-plus-plus/notepad-plus-plus#13330

Closed

Search code, repositories, users, issues, pull requests...

Conversation

alvinhochun commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvinhochun commented Dec 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kennykerr commented Dec 12, 2022

Uh oh!

Uh oh!

Uh oh!

dmachaj commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alvinhochun commented Dec 11, 2022 •

edited

Loading