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

Which MSVC CRT to use? #99

Open
rpavlik opened this issue Aug 2, 2019 · 4 comments

Comments

@rpavlik
Copy link
Contributor

commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

cc @daveh-lunarg for possible insight :)

@brycehutchings

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

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.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

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.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.