This patch corrects the build errors I encountered when building on MinGW64.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
@EricWF and I discussed this on IRC a bit. I'm not a fan of overloading _LIBCPP_WIN32API for this purpose, since to me that macro is meant for guarding Windows API functions, not for CRT functions. Eric suggested adding a new macro _LIBCPP_MSVCRT_LIKE, which I'd be fine with.
I agree that we need to be precise in our platform detection macros. On Windows, we currently need to worry about which compiler is being used, whether we are targeting the mingw environment or the native (?) environment, and which c-runtime is being used.
For my msvc-kernel work, I'll need some amount of platform detection macros as well (I'm not expecting those to be added in this review). I believe I will represent that with some combination of a "freestanding" macro, a different name for the CRT, and detection of the MSVC compiler, but I haven't gotten that far yet.
I don't know if it is MSYS2 specific or general MinGW issue but liblibc++.a is created. Could you check your build?
Here is line to blame: https://github.com/llvm-mirror/libcxx/blob/master/lib/CMakeLists.txt#L246
include/__locale | ||
---|---|---|
370 ↗ | (On Diff #98570) | Are these really Win32? These are really more libc compatibility things I thought? |
include/stdio.h | ||
113 ↗ | (On Diff #98570) | Again, I dont think that this is Win32 API related. This is a libc implementation detail. If I were to build libc++ against musl and Win32, these would be present, right? |
include/wchar.h | ||
169 ↗ | (On Diff #98570) | Again not sure that this is Win32 API specific. |
src/new.cpp | ||
186 ↗ | (On Diff #98570) | This is definitely msvcrt specific. It is possible for an alternate c library to provide definitions here. |
259 ↗ | (On Diff #98570) | Part of the change above. |
src/support/win32/locale_win32.cpp | ||
125 ↗ | (On Diff #98570) | Would you mind using: #if !defined(_LIBCPP_MSVCRT) instead? |
src/system_error.cpp | ||
68 ↗ | (On Diff #98570) | I think that MSVCRT is more appropriate here. |
@compnerd see my previous comment:
@EricWF and I discussed this on IRC a bit. I'm not a fan of overloading _LIBCPP_WIN32API for this purpose, since to me that macro is meant for guarding Windows API functions, not for CRT functions. Eric suggested adding a new macro _LIBCPP_MSVCRT_LIKE, which I'd be fine with.
_LIBCPP_MSVCRT isn't defined for MinGW (since they have their own CRT headers), so we need a new macro that's defined for both MSVCRT and MinGW.
Sure, a _LIBCPP_MSVCRT_LIKE WFM. I just want to make sure that we don''t conflate the underlying libc implementation with the Win32 API set.
Yup, that was my concern as well.
Sounds like a cmake prefix issue. We should be able to fix this by unconditionally setting CMAKE_STATIC_LIBRARY_PREFIX to lib instead of changing the output name, I think.
I want to give some context here to dispel the confusion of what is and isn't win32 api specific.
First lets take vasprintf and asprintf which are not implemented in msvcrt.
In mingw-w64 we would just have posix implementations of both.
They are hidden behind the guard _GNU_SOURCE, we don't need to declare them in include/stdio.h in libcxx but
By default gcc doesn't pass this flag for the mingw target so we should add it in cmake within libc++.
We should be able to guard mingw to stop it from picking up the implementations in windows/support.cpp
Next example mbsnrtowcs and wcsnrtombs above
There is technically a win32api specific implementation in msvc. The mingw-w64 implementation or lack there of would rely on the MSVC implementation.
This is why a custom implementation lives in libc++ in support.cpp that is posix compliant.
Unfortunately a posix implementation might not be accepted upstream into mingw-w64 because they need to maintain compatibility with msvc, even with some bugs eek.
It generally comes down to if a bunch of projects are using it already and we should not disrupt them relying on msvc implementations/bugs.
This is probably not the case for these two functions but there are many others already in the library that follow this guideline.
A good read of this specific bug is here.
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130325/077071.html
@smeenai suggestion of _LIBCPP_MSVCRT_LIKE makes a lot of sense here because mingw-w64 is only partially posixy, this leaves room for future possible targets like midipix which uses musl libc to ignore this section of code where mingw and msvc opt in.
Defining _GNU_SOURCE during the library build isn't enough, it's also has to be defined when people are using the headers,
and in that case we must depend on the compiler to pre-define it. Is it important that MinGW not define _GNU_SOURCE by default?
Also do you know why asprintf is declared by mingw-w64 but vasprintf isn't? At minimum I think we still need to declare vasprintf in the
headers because we can't count on _GNU_SOURCE being defined before <features.h> is first included, but we should be able to omit
the definition.
Next example mbsnrtowcs and wcsnrtombs above
There is technically a win32api specific implementation in msvc. The mingw-w64 implementation or lack there of would rely on the MSVC implementation.
This is why a custom implementation lives in libc++ in support.cpp that is posix compliant.
Good to know.
Unfortunately a posix implementation might not be accepted upstream into mingw-w64 because they need to maintain compatibility with msvc, even with some bugs eek.
I'm not personally concerned about upstreaming MinGW-w64 changes/fixes. If we can adequately solve it within libc++ without any terrible hacks I'm happy with that for now.
It generally comes down to if a bunch of projects are using it already and we should not disrupt them relying on msvc implementations/bugs.
This is probably not the case for these two functions but there are many others already in the library that follow this guideline.
Are you suggesting that libc++ should stop declaring/defining these functions, at least publically?
A good read of this specific bug is here.
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130325/077071.html@smeenai suggestion of _LIBCPP_MSVCRT_LIKE makes a lot of sense here because mingw-w64 is only partially posixy, this leaves room for future possible targets like midipix which uses musl libc to ignore this section of code where mingw and msvc opt in.
! In D33082#764617, @EricWF wrote:
Defining _GNU_SOURCE during the library build isn't enough, it's also has to be defined when people are using the headers,
and in that case we must depend on the compiler to pre-define it. Is it important that MinGW not define _GNU_SOURCE by default?
I'm not sure why this isn't the default, it could be some backwards compatibility with mingw.org.
I will ask kai the author and find out.
Also do you know why asprintf is declared by mingw-w64 but vasprintf isn't? At minimum I think we still need to declare vasprintf in the
headers because we can't count on _GNU_SOURCE being defined before <features.h> is first included, but we should be able to omit
the definition.
I believe it is in stdio.h but behind the guard __USE_MINGW_ANSI_STDIO
You are currently picking up is msvc compatable asprintf and not gnu.
Defining this will give you the gnu versions of both vasprintf, asprintf and also the GNU printf format specifiers.
You should be able use this to build libcxx just don't declare it publicly
Are you suggesting that libc++ should stop declaring/defining these functions, at least publically?
We could in the future run into real world problems with projects that work with libstdc++ fine but will have problems with libc++ if a project implements its own version but I wouldn't be too concerned about this right now because when that becomes a problem it will prompt people to get these into mingw-w64 upstream.
Are you suggesting that libc++ should stop declaring/defining these functions, at least publically?
I think that we generally shouldn't be giving functions names that are already claimed elsewhere (like mbsnrtowcs and wcsnrtombs). It is my opinion that these should always be qualified as __libcpp_* symbols. We can easily run into trouble if users also defined mbsnrtowcs. They have just as much of a right to do so as we do.
That being said, I think that's a change for a different changelist.
Agreed. Libc++ shouldn't be stealing names it doesn't own, but fixing that is a separate change.
Libc++ uses vasprintf in the headers, so it's not just a matter of building the library with _GNU_SOURCE.
Since we can't dependably define _GNU_SOURCE or __USE_MINGW_ANSI_STDIO in the headers we're
eventually going to have to provide our own implementations under different names. But I would rather do that as a separate commit if that's OK.
- remove asprintf declaration and definition entirely. It's not used anywhere within libc++.
LGTM but I can't speak for the area where you added #include <cstdio> and killed off the _NEWLIB_VERSION check
Seems in order based on https://sourceware.org/ml/newlib-cvs/2014-q3/msg00038.html
Maybe make a note of the minimum newlib version supported somewhere?
Others may have more specific thoughts on the diff.
I removed the section you're referring to because it's unneeded. <__locale> does the exact same #include dance. and <locale> includes <__locale>.
It should not have an affect on functionality.
! In D33082#765898, @EricWF wrote:
I removed the section you're referring to because it's unneeded. <__locale> does the exact same #include dance. and <locale> includes <__locale>.
It should not have an affect on functionality.
In that case just ignore my previous comment. LGTM