Stop exporting winrt::impl::get_marshaler to workaround MSVC modules bug

[DefaultRyan]

An MSVC bug was revealed where the combination of export extern "C++", a function-local type in an inline function, and that type having a user-defined constructor, causes that constructed object to have a zero-filled vtable. So calling any virtual function causes a crash.

This affected winrt::impl::get_marshaler(), responsible for constructing the implementation of IMarshal for winrt::implements.

Verified this bug by adding a case to test_cpp20_module that constructs an implements object, and queries for IMarshal. The ensuing call to Release() via that interface causes a crash.

Since winrt::impl::get_marshaler() is an implementation detail that doesn't need to be exported (and we are planning to reduce spurious exports anyway), it makes sense to stop exporting this function. This change is sufficient to workaround the MSVC bug and the test now passes.

Fixes: #1590

[YexuanXiao]

I think there is a certain issue with the current patch. Functions exported by C++/WinRT have extern "C++", which allows them to link with header files and share the same symbols. If you remove extern "C++" from get_marshaler (produced by WINRT_EXPORT), the get_marshaler inside the module will generate its own symbol. As a result, functions exported by the C++/WinRT module will have the same symbols as the header file users, but refer to different symbols of get_marshaler, leading to an ODR violation. I haven't looked into other workarounds in detail yet, but as I mentioned earlier, we might consider separating extern "C++" from WINRT_EXPORT and moving it to the .ixx file, like:

extern "C++" {
#include "winrt/Windows.Foundation.h"
}

Also, keep get_marshaler from being exported.

[sylveon]

I don't think it's an ODR violation. The module's exported symbols will have module linkage, meaning they are not the same symbols. The only place we really need to use extern "C++" is when symbols need to be shared, like the winrt_get_activation_handler and other function pointer interfaces.

[YexuanXiao]

No, extern "C++" and module linkage are unrelated things. The STL wraps everything in std with extern "C++", which is consistent with the current C++/WinRT approach, and that was intentional on my part.

[sylveon]

Note the comment

// In the STL's headers (which might be used to build the named module std), we unconditionally
// and directly mark declarations of our separately compiled machinery as extern "C++", allowing
// the named module to work with the separately compiled code (which is always built classically).

// TRANSITION: _USE_EXTERN_CXX_EVERYWHERE_FOR_STL controls whether we also wrap the STL's
// header-only code in this linkage-specification, as a temporary workaround to allow
// importing the named module in a translation unit with classic includes.

This should not be a problem for us as we don't really support mixing includes and imports. We also do not have much in terms of separately built machinery.

[YexuanXiao]

This should not be a problem for us as we don't really support mixing includes and imports.

In my own fork, I have already implemented support for mixing includes and imports, and C++/WinRT has inherited this as well.