Skip to content
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

Fix invoke call on Clang (possibly a compiler bug) #1225

Merged

Conversation

alvinhochun
Copy link
Contributor

For unknown reasons, Clang resolves the invoke call (in winrt::impl::promise_base::Completed and winrt::impl::promise_base::set_completed) to std::invoke instead of winrt::impl::invoke, which breaks exception handling and causes some disconnected.cpp tests to crash. After changing the call to specify the namespace, 4 more tests in disconnected.cpp now passes on Clang.


To demonstrate the issue, I modified the generated winrt/Windows::Foundation.h and duplicated line 3765 (winrt::impl::promise_base::set_completed), and changed it so that one call specifies the namespace and the other doesn't. The disassembly shows how the name resolution of the two calls differ (this is with clang+mingw but the same happens with clang+msvc):

** 3763             if (handler)
   3764             {

    0x1ea95c <+156>: leal   -0x8(%ebp), %ecx
    0x1ea95f <+159>: calll  0x155e70                  ; winrt::Windows::Foundation::IUnknown::operator bool at base.h:2139
    0x1ea964 <+164>: testb  $0x1, %al
    0x1ea966 <+166>: jne    0x1ea971                  ; <+177> at Windows.Foundation.h
    0x1ea96c <+172>: jmp    0x1ea9a7                  ; <+231> at Windows.Foundation.h:3768:9

   1    // WARNING: Please don't edit this file. It was generated by C++/WinRT v2.3.4.5

    0x1ea971 <+177>: movl   -0x14(%ebp), %ecx

-> 3765                 invoke(handler, *this, status);

->  0x1ea974 <+180>: movl   %esp, %eax
    0x1ea976 <+182>: leal   -0xc(%ebp), %edx
    0x1ea979 <+185>: movl   %edx, 0x8(%eax)
    0x1ea97c <+188>: movl   %ecx, 0x4(%eax)
    0x1ea97f <+191>: leal   -0x8(%ebp), %ecx
    0x1ea982 <+194>: movl   %ecx, (%eax)
    0x1ea984 <+196>: calll  0x1ea9c0                  ; std::__1::invoke<winrt::Windows::Foundation::AsyncActionWithProgressCompletedHandler<int>&, winrt::impl::promise_base<std::__1::coroutine_traits<winrt::Windows::Foundation::IAsyncActionWithProgress<int>>::promise_type, winrt::Windows::Foundation::IAsyncActionWithProgress<int>, int>&, winrt::Windows::Foundation::AsyncStatus&> at invoke.h:530
    0x1ea989 <+201>: jmp    0x1ea98e                  ; <+206> at Windows.Foundation.h

   1    // WARNING: Please don't edit this file. It was generated by C++/WinRT v2.3.4.5

    0x1ea98e <+206>: movl   -0x14(%ebp), %ecx

** 3766                 ::winrt::impl::invoke(handler, *this, status);
   3767             }

    0x1ea991 <+209>: leal   -0x8(%ebp), %edx
    0x1ea994 <+212>: leal   -0xc(%ebp), %eax
    0x1ea997 <+215>: movl   %edx, (%esp)
    0x1ea99a <+218>: movl   %ecx, 0x4(%esp)
    0x1ea99e <+222>: movl   %eax, 0x8(%esp)
    0x1ea9a2 <+226>: calll  0x1ea9f0                  ; winrt::impl::invoke<winrt::Windows::Foundation::AsyncActionWithProgressCompletedHandler<int>, winrt::impl::promise_base<std::__1::coroutine_traits<winrt::Windows::Foundation::IAsyncActionWithProgress<int>>::promise_type, winrt::Windows::Foundation::IAsyncActionWithProgress<int>, int>, winrt::Windows::Foundation::AsyncStatus> at base.h:5859

** 3768         }

For unknown reasons, Clang resolves the `invoke` call to `std::invoke`,
which breaks exception handling and causes some disconnected.cpp tests
to crash.
@alvinhochun alvinhochun marked this pull request as ready for review November 7, 2022 12:17
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

Sweet, thanks!

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kennykerr kennykerr merged commit 526d72a into microsoft:master Nov 7, 2022
@alvinhochun alvinhochun deleted the alvin/clang-invoke-error-fix branch December 11, 2022 16:47
@dmachaj
Copy link
Contributor

dmachaj commented Dec 14, 2023

This crash has started affecting some of our components that are on an older cppwinrt version. For unknown reasons std::invoke is now called by MSVC in addition to the previous Clang problems. This manifests as a crash.

Fortunately the fix is available for more than a year. We just need to update cppwinrt versions in a lot more places to get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants