OpenXR-SDK-Source icon indicating copy to clipboard operation
OpenXR-SDK-Source copied to clipboard

Which MSVC CRT to use?

Open rpavlik opened this issue 4 years ago • 6 comments

I've seen some concerns (including my own) about the current CRT choice on MSVC for the loader:

  • If built as static lib (default on Windows), the dynamic CRT is used.
  • If build as a DLL (non-default, and possibly not tested because I'm pretty sure the header is missing some dllimport and/or manually import getinstanceprocaddr and build your own dispatch table), the static CRT is used

(see for example https://github.com/microsoft/vcpkg/blob/master/ports/openxr-loader/portfile.cmake#L22 )

Can anyone fill me in (and possibly the docs) on what the rationale is for this behavior? Does it make sense to change it, or allow it to be configurable?

rpavlik avatar Aug 02 '19 18:08 rpavlik

cc @daveh-lunarg for possible insight :)

rpavlik avatar Aug 02 '19 18:08 rpavlik

Just my guesses:

For building as a DLL, the rationale is that it removes any dependencies so that it "just works". The downside is the DLL is bloated and could potentially bake in CRT vulnerabilities.

For building as a static lib, it's probably more arbitrary. At least on the Windows UWP platform, static linking of CRT is not even an option so building the lib with the dynamic CRT is the only correct option. To be honest, I'm not sure how successful one can build a .lib with one toolchain/build configuration and use it in another. Different compiler settings like around exception handling may make the .lib incompatible.

I don't think there can be correct defaults (especially for the static lib case) so more configuration sounds like a good thing, and might make integration into tools like vcpkg more flexible.

brycehutchings avatar Aug 02 '19 18:08 brycehutchings

This was a change from previous always-dynamic linking, in PR #7. My recollection is this was a request from a WG discussion just before GDC, and the consensus was this was the least bad combination. I can't reconstruct the rationale used but I think it's pretty much what Bryce said.

I'm also in favor of adding a cmake switch to make it configurable.

daveh-lunarg avatar Aug 05 '19 19:08 daveh-lunarg

Thanks for the feedback. I'd also like to use BUILD_SHARED_LIBS (the standard way of telling CMake to make a .DLL instead of a .LIB) instead of DYNAMIC_LOADER, but that would change how the build is consumed, which might be a non-starter?

I'd suggest we look at how vcpkg typically indicates it wants various runtime libs, etc. and set things up so that it works right "by default" (e.g. uses the "standard" names for variables)

rpavlik avatar Aug 13 '19 19:08 rpavlik

I'd suggest we look at how vcpkg typically indicates it wants various runtime libs, etc. and set things up so that it works right "by default" (e.g. uses the "standard" names for variables)

This is a good talk by one of the vcpkg devs at CPPCON 2018 that talks about this in detail: https://www.youtube.com/watch?v=sBP17HQAQjk

The bit about using the cmake standard functionality is around 7 minutes in.

jherico avatar Sep 16 '19 01:09 jherico

An issue (number 1287) has been filed to correspond to this issue in the internal Khronos GitLab.

If you have a Khronos account, you can access that issue at KHR:openxr/openxr#1287.

rpavlik-bot avatar Dec 12 '19 18:12 rpavlik-bot