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

Support building with MSVC #151

Merged
merged 18 commits into from
Jan 19, 2024
Merged

Support building with MSVC #151

merged 18 commits into from
Jan 19, 2024

Conversation

ishitatsuyuki
Copy link
Contributor

This is a series to support building dxvk-nvapi on MSVC for fun, diversity and binary size reduction.
In addition to MSVC, cross compiling from Linux with clang-cl is also supported using https://github.com/mstorsjo/msvc-wine and a cross file like https://gist.github.com/ishitatsuyuki/bfa4b4764394f94b19115b182d269f4e.

Most changes are described in the commit message. This is a list of major breaking changes for those who need to rebase:

  • When declaring a new extension interface, declare the GUID in the header using __CRT_UUID_DECL instead of FOO_GUID. The .cpp files are deleted and the guid field no longer needs to be assigned manually.
  • When writing tests, use __uuidof instead of ::guid or IID_.
  • When introducing new D3D12 mocks, methods than return a struct by value will need different mocks for MinGW and MSVC. See tests: Make mocks compatible with MSVC COM return ABI for an example.

strcasecmp is a GNU extension not supported by MSVC. On the other hand the
_stricmp name is supported by MinGW. Since our code targets Windows it
should be fine to use the MSVC name.
From VS2015+, the subsystem headers fails with "No target architecture"
because the target definitions are declared only in windows.h.
We only have those includes in one file, so it seems fine to do this.
Our in-tree header is for MinGW and doesn't compile on MSVC. Recent Windows
SDK ships headers with the DX12 extensions we need, so unlike MinGW we
should not need to use a vendored version.
@ishitatsuyuki
Copy link
Contributor Author

Actually, forgot one change to add dllexport to nvapi_queryinterface. One sec.

@ishitatsuyuki ishitatsuyuki marked this pull request as draft January 19, 2024 10:08
@ishitatsuyuki ishitatsuyuki marked this pull request as ready for review January 19, 2024 10:12
Copy link
Collaborator

@Saancreed Saancreed left a comment

Choose a reason for hiding this comment

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

Reasonable changes. WIDL_EXPLICIT_AGGREGATE_RETURNS thingy is a bit annoying but I suppose we can live with it. Just one minor request and one question, LGTM otherwise.

src/dxvk/dxvk_interfaces.h Outdated Show resolved Hide resolved
src/nvapi_interface.cpp Show resolved Hide resolved
Copy link
Owner

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

Thanks a lot, also for fixing a few things in between. Looks really good. See a few questions.

meson.build Show resolved Hide resolved
tests/nvapi_d3d12_mocks.h Outdated Show resolved Hide resolved
.github/workflows/test-build-windows.yml Outdated Show resolved Hide resolved
The canonical ABI for COM method calls that return a structure by value is
to "pass a hidden first parameter that is the address of an uninitialized
structure in which to place the result" [1]. As GCC does not conform to
this convention, MinGW goes through length to workaround the ABI issue by
adding a wrapper that explicitly takes the hidden return value parameter.

This hack is unnecessary on MSVC. Unfortunately, it also means that we need
to declare mocks with different argument count. Use ifdef against
WIDL_EXPLICIT_AGGREGATE_RETURNS (the macro defined by MinGW to enable the
wrappers) to switch to the appropriate version. For MSVC, there are no
overloads coming from the ABI hack, so we can use the simpler
IMPLEMENT_MOCK macros here.

The luid return was also technically wrong (it should write to the pointer
argument, not return an unrelated address), but the bug was masked by the
MinGW wrapper. Fix it, just in case.

[1]: https://devblogs.microsoft.com/oldnewthing/20220113-00/?p=106152
get_supported_arguments will discard the unsupported arguments, however
clang-cl recognizes gcc-style arguments too and ends up taking unnecessary
arguments.
These are macros commonly used to configure windows.h's behavior.
NOMINMAX suppresses the definition of min and max to prevent collision with
standard library functions.
WIN32_LEAN_AND_MEAN skips definitions of rarely used Win32 APIs to speed up
compilation.
Replace ::guid fields with the more standard __uuidof builtin. On MinGW,
the GUIDs need to be associated with the interface using __CRT_UUID_DECL.
This is not necessary for MSVC which recognize the declspec(uuid) from
MIDL_INTERFACE.
The IID_ symbols requires linking to dxguid.lib on MSVC. This is not
necessary if __uuidof is used.
Designated initializers require C++20. This was supported by GCC and Clang
in older versions as vendor extensions, but not MSVC.
@ishitatsuyuki ishitatsuyuki force-pushed the msvc branch 3 times, most recently from 53854d4 to 45bce23 Compare January 19, 2024 14:47
clang-cl (MSVC mode) does not define __GNUC__, but still uses
GCC-compatible diagnostic flags.

Also add missing guard in nvapi_tests_private.h to suppress MSVC warnings
about unknown pragma.
For now, suppress these warnings.
The C++ standard does not allow zero length arrays. Additionally, trying to
initialize an unknown-length array with an empty initializer is treated as
VLA in MSVC, which is also unsupported.

Explicitly initializing as nullptr seems reasonable here.
If the variable is not const or constexpr MSVC complains that the array is
a VLA which is unsupported. Instead, initialize the array with a constant
length first, then use std::size (C++17 alternative to _countof) to
retrieve the array length.
VLAs are not supported by MSVC.
This function has to be exported.
Copied mostly verbatim from DXVK.
Also rename the Linux (MinGW) job for clarity.
@jp7677 jp7677 merged commit ad2d3f9 into jp7677:master Jan 19, 2024
2 checks passed
@ishitatsuyuki
Copy link
Contributor Author

Thanks for the quick review!

@ishitatsuyuki ishitatsuyuki deleted the msvc branch January 19, 2024 15:08
@jp7677 jp7677 mentioned this pull request Jan 19, 2024
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.

4 participants