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

Update the mingw32uwp wrapper libraries #127

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Jun 2, 2020

  • windowsapp is the preferred library when targeting Windows 10, it's safe to use it with the current mingw-w64 repo
    https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis

  • ucrtbase.dll is the DLL that holds the equivalent of what was in vcruntime140_app

  • __MSVCRT_VERSION__ ensures we use the Universal C runtime

@Biswa96
Copy link

Biswa96 commented Jun 2, 2020

Forcing __MSVCRT_VERSION__ seems to be specific. How about adding a #define?

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 2, 2020

Sure

  • ucrtbase.dll is the DLL that holds the equivalent of what was in vcruntime140_app

Are you sure that it is allowed to link against ucrtbase.dll in UWP mode? Because when I looked into this (2 years ago?) the App Certification Kit explicitly pointed out that it is not allowed to link against ucrtbase.dll in such apps, only the indvidual api-ms-win-crt-*.dll (and not the -private one). That's why I added libucrt.a targeting the api-ms-win-crt*.dll at all.

I rechecked now in App Certification Kit/SupportedAPIs-x64.xml from WinSDK 10.0.18362.0, and there's no ucrtbase.dll mentioned anywhere there.

  • __MSVCRT_VERSION__ ensures we use the Universal C runtime

I prefer _UCRT instead of using this define here. Yes it's a bikeshed issue, but here I choose the color :P

We could add that define, but it doesn't really help the fact that libc++ isn't really functional if it was built originally against msvcrt.dll but you want to link against ucrt. That's why I have a check for the default CRT in https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L38-L40, and if it isn't UCRT, we just skip testing the uwp variant of the toolchain.

I guess an even more correct approach would to simply leave out the -uwp frontend wrappers if the toolchain wasn't built in UCRT mode?

Forcing __MSVCRT_VERSION__ seems to be specific. How about adding a #define?

UWP explicitly requires you to use UCRT, so the intent of this is entirely correct. But I'm not entirely convinced that it's worth it as it doesn't fully work if it wasn't the default.

@robUx4
Copy link
Contributor Author

robUx4 commented Jun 3, 2020

  • ucrtbase.dll is the DLL that holds the equivalent of what was in vcruntime140_app

Are you sure that it is allowed to link against ucrtbase.dll in UWP mode? Because when I looked into this (2 years ago?) the App Certification Kit explicitly pointed out that it is not allowed to link against ucrtbase.dll in such apps, only the indvidual api-ms-win-crt-.dll (and not the -private one). That's why I added libucrt.a targeting the api-ms-win-crt.dll at all.

I'm not sure yet but I will give it a try on the Xbox. I ended up looking at this because mixing ucrt and vcruntime140_app seemed odd. I just wanted to get rid of vcruntime140_app since we have ucrt now (not at the time we did UWP for Win8). Removing it ended up linking to api-*-private-*.dll DLLs which seemed wrong as well. So after removing (locally) these DLLs from ucrt I got a few missing C runtime functions that were missing like strstr. And I found them in ucrtbase which seems to be the proper DLL to use.

I rechecked now in App Certification Kit/SupportedAPIs-x64.xml from WinSDK 10.0.18362.0, and there's no ucrtbase.dll mentioned anywhere there.

I also did the same check. Neither are vcruntime140_app.dll nor api-*-private-*.dll are listed in there either. So a few C Runtime entries needed by the VLC Core would be missing.

  • __MSVCRT_VERSION__ ensures we use the Universal C runtime

I prefer _UCRT instead of using this define here. Yes it's a bikeshed issue, but here I choose the color :P

OK

We could add that define, but it doesn't really help the fact that libc++ isn't really functional if it was built originally against msvcrt.dll but you want to link against ucrt. That's why I have a check for the default CRT in https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L38-L40, and if it isn't UCRT, we just skip testing the uwp variant of the toolchain.

I guess an even more correct approach would to simply leave out the -uwp frontend wrappers if the toolchain wasn't built in UCRT mode?

That sounds like a good idea.

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 3, 2020

  • ucrtbase.dll is the DLL that holds the equivalent of what was in vcruntime140_app

Are you sure that it is allowed to link against ucrtbase.dll in UWP mode? Because when I looked into this (2 years ago?) the App Certification Kit explicitly pointed out that it is not allowed to link against ucrtbase.dll in such apps, only the indvidual api-ms-win-crt-.dll (and not the -private one). That's why I added libucrt.a targeting the api-ms-win-crt.dll at all.

I'm not sure yet but I will give it a try on the Xbox.

That might succeed even if it isn't allowed by a validity check (although I don't really know anything about Xbox).

I ended up looking at this because mixing ucrt and vcruntime140_app seemed odd. I just wanted to get rid of vcruntime140_app since we have ucrt now (not at the time we did UWP for Win8). Removing it ended up linking to api-*-private-*.dll DLLs which seemed wrong as well. So after removing (locally) these DLLs from ucrt I got a few missing C runtime functions that were missing like strstr. And I found them in ucrtbase which seems to be the proper DLL to use.

Well not really.

ucrtbase.dll is the physical DLL that contains the actual implementation of code, just like kernel32.dll and all that. But when targeting UWP, you aren't supposed to link directly against kernel32.dll, you link against windowsapp, which then ends up importing api-ms-win-core-*.dll. If there are functions from kernel32.dll, that aren't available in api-ms-win-core-*.dll, you can't just link against kernel32.dll because it happens to have the functions you want. The same goes for UCRT, with ucrtbase.dll vs api-ms-win-crt-*.dll (except for the -private one).

If you build an UWP app with MSVC, it ends up linking against api-ms-win-crt-* only, but also vcruntime140_app.dll - but not ucrtbase.dll. Vcruntime140_app.dll isn't listed in App Certification Kit though, because it isn't a system DLL - when you build an UWP app with MSVC, this DLL gets bundled with your app.

So if you build a DLL component (e.g. libvlc) for use within an app packaged by MSVC (like vlc-winrt today), linking against vcruntime140_app is the correct, and simplest, thing to do. That's where we are right now.

If you don't want to package the app with MSVC and don't want to bundle vcruntime140_app.dll, you'll need to provide your own version of these functions, like strstr and memcpy, statically linked (or as a DLL that you bundle in your own app), just like we already discussed in #109. Musl probably is a good source for suitably licensed implementations that one could use for making such a CRT supplement library for these few functions.

@robUx4
Copy link
Contributor Author

robUx4 commented Jun 3, 2020

Some background on the various C Runtimes available in recent MSVC: https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features and also https://devblogs.microsoft.com/cppblog/introducing-the-universal-crt/

From MSVC but from the documentation it seems it will end up using ucrtbase.dll and not the actual api-ms-win-crt-*.dll.

The blog post is interresting as it describes what you need to use when using /nodefaultlib.

But I just tried building a blank app in VS 2019 with /MD and looking at the DLL dependencies it doesn't use ucrtbase.dll at all, as you said. There are the api-ms-win-crt-*.dll but also vcruntime140_app.dll and vcruntime140_1_app.dll. So I guess we're stuck with it.

@robUx4
Copy link
Contributor Author

robUx4 commented Jun 3, 2020

Also there is no ucrtbase.lib in VS2019 so it doesn't seem right to link with it anyway.

- windowsapp is the preferred library when targeting Windows 10
https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis

- _UCRT ensures we use the Universal C runtime
@mstorsjo
Copy link
Owner

mstorsjo commented Jun 3, 2020

I have an upcoming PoC patch for getting rid of vcruntime140_app, it's ready for posting within a few days.

@mstorsjo mstorsjo merged commit 26e56ed into mstorsjo:master Jun 3, 2020
@mstorsjo
Copy link
Owner

mstorsjo commented Jun 3, 2020

See https://github.com/mstorsjo/mingw-w64/commits/ucrt-app for a PoC for getting rid of vcruntime140_app.dll, in particular, mstorsjo/mingw-w64@1fdf0a5.

@robUx4 robUx4 deleted the uwp-libs branch June 4, 2020 05:46
@robUx4
Copy link
Contributor Author

robUx4 commented Jun 4, 2020

Interresting. I also started a separate lib based on musl. Adding more CRT code in mingw-w64 means more code to maintain there. In my case it's just the musl source code where I cherry pick the files I want to build (there's no autotools/cmake/meson in there so it's easy to just add one in there without patching anything).

Also IMO the libraries to include should have the same names as in MSVC so probably vcruntime (and maybe cmt as well) which would be included with ucrt. But that means removing the *-private-*.dll entries from ucrt. Which might be in contradiction with how it's generated with gendef (if I understood correctly).

Maybe we should move this discussion on the mingw-w64 ML to see what others think.

@robUx4
Copy link
Contributor Author

robUx4 commented Jun 4, 2020

(just noticed some C files come directly from musl so it sounds good, apart from the naming&splitting of libs)

@mstorsjo
Copy link
Owner

mstorsjo commented Jun 4, 2020

Interresting. I also started a separate lib based on musl. Adding more CRT code in mingw-w64 means more code to maintain there. In my case it's just the musl source code where I cherry pick the files I want to build (there's no autotools/cmake/meson in there so it's easy to just add one in there without patching anything).

Well it's not just the musl source files. If looking at what functions there are in vcruntime140_app or api-ms-win-crt-private, it's primarily a lot of windows specific functions (although __C_specific_handler is the only one that ends up called normally), windows specific implementations of setjmp/longjmp, and a handful of string.h functions (a couple mem*, str* and wcs* functions).

mingw-w64-crt already does have a number of crt implementation files (math routines, stdio implementations, etc), so in general I don't think the maintainance of these few is a big thing in itself. The only issue is that memcpy/memmove are pretty performance sensitive, and having well optimized implementations for them can be pretty important. (musl does seem to have some asm implementation of it, but I don't know how it stacks up against e.g. glibc's, or how good the ones in msvcrt.dll or ucrtbase.dll are either.)

The goal was to have all of this work out of the box by just building mingw-w64-crt, as it doesn't seem like much of a stretch from where we are today, contrary to adding yet another third party repo to the build.

Also IMO the libraries to include should have the same names as in MSVC so probably vcruntime (and maybe cmt as well) which would be included with ucrt. But that means removing the *-private-*.dll entries from ucrt. Which might be in contradiction with how it's generated with gendef (if I understood correctly).

In general, matching lib names with MSVC is desireable, for the cases where it makes sense, but the CRT is hooked up quite differently in mingw vs MSVC anyway. I wouldn't try naming anything of this cmt because it's really far off from being that.

You don't need to worry about gendef - while def files might have been generated with such a tool originally, they're maintained by hand once they are in mingw-w64; many of them have manual edits, or manual ifdefs for symbols that are conditional for some architectures.

In MSVC you normally link one toplevel library (msvcrt.lib or libcmt.lib) which then implicitly link in the other necessary ones (ucrt.lib and vcruntime.lib). In mingw, you have libmsvcrt.a which is the default implicitly linked one, which can be a copy of either libmsvcrt-os.a, libucrtbase.a or libucrt.a. Clang also has a rule, that if you manually add -lucrt* or -lmsvcr*, it will drop the default -lmsvcrt. So that does allow us to have the normal libmsvcrt.a be what it is today, equal to libucrt.a, linking against api-ms-win-crt-private-*.dll, but if you add -lucrtapp (via the uwp wrapper script) it instead chooses this implementation.

That lets the default be what it is today (with no compromises regarding implementation of these functions), while -lucrtapp gets a UWP-friendly version (where the implementations I posted so far are a bit stripped down, which should be just fine for our usecase, but could be improved over time as a nice-to-have). And you can switch between all the -lucrt* variants at link time as long as the libc++ has been built against UCRT headers.

Maybe we should move this discussion on the mingw-w64 ML to see what others think.

I guess that'd be a good thing to do yes. I could post my PoC there and see what others think.

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.

None yet

3 participants