Page MenuHomePhabricator

Fix Libc++ build with MinGW64
ClosedPublic

Authored by EricWF on May 10 2017, 5:45 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.May 10 2017, 5:45 PM
smeenai edited edge metadata.May 10 2017, 11:19 PM

@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.

bcraig edited edge metadata.May 11 2017, 8:26 AM

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.

mati865 accepted this revision.May 14 2017, 3:12 AM

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

This revision is now accepted and ready to land.May 14 2017, 3:12 AM
compnerd requested changes to this revision.May 14 2017, 12:33 PM
compnerd added inline comments.
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.

This revision now requires changes to proceed.May 14 2017, 12:33 PM

@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.

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.

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

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.

martell added a comment.EditedMay 21 2017, 6:00 PM

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.

EricWF marked 6 inline comments as done.May 25 2017, 10:04 AM
EricWF added inline comments.
include/__locale
370 ↗(On Diff #98570)

So _LIBCPP_MSVCRT_LIKE?

include/stdio.h
113 ↗(On Diff #98570)

Changing to _LIBCPP_MSVCRT_LIKE.

include/wchar.h
169 ↗(On Diff #98570)

Changed to _LIBCPP_MSVCRT_LIKE.

src/system_error.cpp
68 ↗(On Diff #98570)

Changed to MSVCRT_LIKE.

EricWF marked 3 inline comments as done.May 25 2017, 10:16 AM

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++.

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.

EricWF updated this revision to Diff 100263.May 25 2017, 10:24 AM
EricWF edited edge metadata.

Add _LIBCPP_MSVCRT_LIKE and use it to replace _LIBCPP_WIN32API where appropriate.

martell added a comment.EditedMay 25 2017, 4:44 PM

! 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.

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.

! 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

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.

EricWF updated this revision to Diff 100358.May 25 2017, 8:30 PM
  • 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.

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?

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.

martell added a comment.EditedMay 26 2017, 10:58 AM

! 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

smeenai accepted this revision.May 29 2017, 5:35 PM

LGTM. Thanks for all the revisions!

This revision was automatically updated to reflect the committed changes.