Changing linker order will cause tests to fail

[tiagomacarios]

Change cpplatest source listing to:

target_sources(witest.cpplatest PUBLIC
    ${CMAKE_CURRENT_SOURCE_DIR}/../CommonTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../ComTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../FileSystemTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../main.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../ResourceTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../ResultTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../SafeCastTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../WistdTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../wiTest.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../CppWinRTTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../StlTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../TokenHelpersTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../UniqueWinRTEventTokenTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../WatcherTests.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/../WinRTTests.cpp
    )

compile and run tests. It will fail.

[dunhor]

Can you share what compiler/architecture/build type you are seeing this with? I tried a few configurations and the tests ran without issue.

Also the errors that you do see, or a sampling of the errors, would be useful as well

[tiagomacarios]

Just vanilla VS2017. I guess I should re-phrase the bug and say that the test executable actually crashes. I don't think any of the bat scripts currently look for the return code. Have you tried executing it manually?

C:\temp>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27031.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

C:\temp>git clone https://github.com/microsoft/wil.git
Cloning into 'wil'...
remote: Enumerating objects: 52, done.
remote: Counting objects: 100% (52/52), done.
remote: Compressing objects: 100% (35/35), done.
remote: Total 189 (delta 17), reused 23 (delta 11), pack-reused 137
Receiving objects: 100% (189/189), 599.75 KiB | 18.17 MiB/s, done.
Resolving deltas: 100% (59/59), done.

C:\temp>cd wil

C:\temp\wil>mkdir build

C:\temp\wil>cd build

C:\temp\wil\build>vim ..\tests\cpplatest\CMakeLists.txt

C:\temp\wil\build>cmake -G"Ninja" ..
-- The C compiler identification is MSVC 19.16.27031.1
-- The CXX compiler identification is MSVC 19.16.27031.1
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: C:/temp/wil/build

C:\temp\wil\build>cmake --build .
[56/56] Linking CXX executable tests\cpplatest\witest.cpplatest.exe

C:\temp\wil\build>tests\cpplatest\witest.cpplatest.exe

C:\temp\wil\build>echo %Errorlevel%
-2147467259

[dunhor]

The scripts check the return code, and I know that it works since I discovered an issue with running the tests on the CI machines by them failing because of it. But anyway I was invoking them manually and on top of having a post-mortem debugger set up, the tests also print out a summary when they complete, so they aren't crashing for me. Could you run the tests under a debugger to see where they crash? That'll likely give the most info.

One obvious difference is that you're using VS 2017. I'll try setting up VS 2017 on a separate machine to see if I get the same behavior. Otherwise, the host OS may play a role (though I'd like to believe that the link order wouldn't have an impact there...)

[dunhor]

Okay, I can hit this now. Seems to be happening in CppWinRTTests::CppWinRTToWilExceptionTranslationTest. Curious that link order is affecting it... Looking into it

[tiagomacarios]

Let me know! I am also curious. Thanks for having a look!

[dunhor]

Ahh, okay, I see now. This is a known possible ODR violation (and in this case is an ODR violation). More info below for those interested. The TL;DR is that this is in the process of being cleaned up, but until then, wil/cppwinrt.h must be included before both wil/result_macros.h as well as winrt/base.h in any project that wants the two to cooperate well.

Basically the "old" - and I say old even though it's still technically current - way to inject your own exception translation logic in C++/WinRT was to define WINRT_EXTERNAL_CATCH_CLAUSE. Similarly, WIL has a check for __WIL_CPPWINRT_INCLUDED in result_macros.h that does similar things. There used to be an ODR violation check (via pragma detect_mismatch) to detect scenarios where the include order requirements were violated, but this proved to be too painful since there were many libs produced with no knowledge of C++/WinRT, so it was removed. Pretty ironic eh, since the result is that issues like this are possible and they have popped up in the past.

This is why C++/WinRT "2.0" now has a global function pointer - winrt_to_hresult_handler - instead of relying on macros and include order. WIL just hasn't caught up just yet to match in this behavior. @kennykerr may have more information on whether or not there have been any discussions recently about modifying WIL to match.

I suppose that if we were motivated to fix this all of the test files could be updated to conditionally include wil/cppwinrt.h first. That said, while I'm not super happy about the tests relying on UB, I'm also not personally all that motivated to go and update it given that it's worked for this long and will eventually no longer be necessary.

[tiagomacarios]

how does the header inclusion affects the linker?

[dunhor]

It's an ODR violation - you have two different implementations of a function with the same signature. E.g. in this case ResultFromCaughtExceptionInternal has two definitions: one with the C++/WinRT exception handling logic and one without. If not inlined, which in this case I believe is likely always since the function is called through a function pointer, the linker chooses one, which appears to be the first one it encounters, and the others are discarded. Thus, if CppWinRTTests.cpp is not linked in first, I'd expect that you'd hit this issue.

[dunhor]

Thinking about it more, moving the C++/WinRT tests to be its own self-contained test (e.g. witest.cppwinrt) is a reasonable solution as well

[ChrisGuzak]

@dunhor, we have 21168025 tracking this. I assume that will fix this once and for all.

[dunhor]

Seems like it. The usage needs to be conditional right now, but I'm not entirely sure that's possible. Kenny is out today so I'll try and sync up with him tomorrow. I know that C++/WinRT has CPPWINRT_VERSION, but that's a string and I don't believe that it's usable with the pre-processor.