This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [libcxxabi] fix building with libc++ win32 threads for mingw-w64
AbandonedPublic

Authored by martell on May 21 2017, 6:04 AM.

Details

Reviewers
EricWF
chapuni
rnk
Summary

When bootstrapping with _LIBCPP_HAS_THREAD_API_WIN32 we need to set windows version to atleast 0x0600 for the fibersapi
We do not use these newer apis with the pthread version, this should be safe to do here regardless.

This is useful for environments that want c++11 threading but without using winpthreads.

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.May 21 2017, 6:04 AM
EricWF added inline comments.May 25 2017, 12:05 AM
libcxxabi/trunk/cmake/config-ix.cmake
42

Same comment about -D_WIN32_WINNT=0x0600 as in D33388.

@EricWF might make the most sense to have the review for -D_WIN32_WINNT=0x0600 here.

committed the iconv removal in rL304026

martell updated this revision to Diff 100459.May 26 2017, 12:51 PM

updated to just contain win32 thread api fix

martell retitled this revision from [libcxxabi] fixup bootstrapping for mingw-w64 to [libcxx] [libcxxabi] fix building with libc++ win32 threads for mingw-w64.May 26 2017, 12:52 PM
martell edited the summary of this revision. (Show Details)
martell added reviewers: chapuni, rnk.
martell removed a subscriber: chapuni.

@EricWF I believe your concern was that 0x600 was Vista.
I only updated this to the min required version to build.
https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/fibersapi.h#L17

EricWF edited edge metadata.May 26 2017, 1:59 PM

Why do we *need* to set it? Doesn't _WIN32_WINNT default to the current platform?

libcxx/trunk/cmake/config-ix.cmake
53

Shouldn't we use add_definitions ?

Why do we *need* to set it? Doesn't _WIN32_WINNT default to the current platform?

Not for mingw-w64, that wouldn't make much sense in general because of cross compiling.
It defaults to` 0x502` here which is a bump above Windows XP, which support was dropped for when MS officially dropped support for that platform.
https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/_mingw.h.in#L225

libcxx/trunk/cmake/config-ix.cmake
53

That makes sense.
I can change it to add_definitions

martell updated this revision to Diff 100569.May 28 2017, 2:31 PM

updated to add_definitions

EricWF requested changes to this revision.May 31 2017, 3:28 PM

This isn't a solution to the problem. It may work when compiling the library, but that's not the important part.
The bits of libc++ that use the fibers interface are in the headers, that means that _WINNT_VER must
also have the same value any time a libc++ header is included by a user. However this can only be
achieved by making the compiler pre-define _WINNT_VER, or by removing the nonsensically old
definition MinGW provides. Neither of these can be done by libc++.

For example:

#include <windows.h> // _WINNT_VER isn't yet defined. Fibers are not included
#include <thread> // libc++ is too late to change the definition.
// ERROR <thread> cannot be compiled.
This revision now requires changes to proceed.May 31 2017, 3:28 PM

Yeah, after discussing this further this is MinGW's problem to fix.

MinGW is *WRONG* for defaulting the macro to such a low value. It should be defaulting it to the value of the target, not 0x502.
https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/_mingw.h.in#L225

martell abandoned this revision.Nov 28 2017, 11:51 PM

The default is now configurable in mingw-w64