This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] fixup bootstrapping for mingw-w64
ClosedPublic

Authored by martell on May 21 2017, 11:46 AM.

Details

Summary

This can technically be split into different commits.
I just wanted to get a review of them together to move things along

The first part is the same as what is in D33384 so once that is reviewed and accepted this should be approved also.

The change to the CMakeLists.txt is so that we get libc++ and not liblibc++ for mingw which is silly.
I am open to an alternative way to achieve this if this isn't a good solution.

Lastly is a header include fixup, mingw from linux doesn't like <Windows.h>
Throughout all llvm projects we use a lowercase w because of this, see rL302340 for reference.

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.May 21 2017, 11:46 AM
chapuni added inline comments.
libcxx/trunk/lib/CMakeLists.txt
246 ↗(On Diff #99700)

"MSVC" is available.

martell marked an inline comment as done.May 21 2017, 4:20 PM
martell added inline comments.
libcxx/trunk/lib/CMakeLists.txt
246 ↗(On Diff #99700)

Yes, We can use MSVC

I have a habit of avoiding it because CMAKE is not great in differentiating between the IDE and the compiler but it should work fine here.
I'm hoping more recent versions detect clang tageting msvc not in cl.exe mode picks that up correctly.
Good spot btw.

martell updated this revision to Diff 99709.May 21 2017, 4:35 PM
martell marked an inline comment as done.
martell added inline comments.
libcxx/trunk/lib/CMakeLists.txt
246 ↗(On Diff #99700)

Now that I think about it more, I would like to avoid this.
Will update to just override CMAKE_STATIC_LIBRARY_PREFIX to lib
We need this to avoid the same name as msvc libc++ I believe.

EricWF added inline comments.May 25 2017, 12:04 AM
libcxx/trunk/cmake/config-ix.cmake
53

-D_WIN32_WINNT=0x0600. What? Why?

First isn't 0x0600 Windows Vista? Why are we asking to support such an old operating system?
Why are we defining this at all?

chapuni added inline comments.May 25 2017, 2:44 AM
libcxx/trunk/cmake/config-ix.cmake
53

I think it'd be good thing to require minimum version as the bottom line.

EricWF added inline comments.May 25 2017, 3:36 AM
libcxx/trunk/cmake/config-ix.cmake
53

Perhaps, but at minimum this is not the patch to do it in unless it's a required change to make MinGW work, otherwise it should be done as a separate commit because a wider range of Win32 interfaces are used when targeting non-MinGW Windows.

martell added inline comments.May 25 2017, 4:46 AM
libcxx/trunk/cmake/config-ix.cmake
53

Yes this patch is required to make mingw work for the win32 c++ threading api in libcxx.
0x0600 is the min version to access the fibersapi.
See here https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/include/fibersapi.h#L17

The default value for mingw-w64 currently is 0x502 See here https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/_mingw.h.in#L225

When building for the pthread api 0x0502 is fine, we just need a bump for fibersapi.

From my description in the other patch Revision.
When bootstrapping with _LIBCPP_HAS_THREAD_API_WIN32 we need to set windows version to atleast 0x0600 for the fibersapi

martell added inline comments.May 25 2017, 4:48 AM
libcxx/trunk/include/__threading_support
32

This is the line related to the comments above

EricWF added inline comments.May 25 2017, 11:12 AM
libcxx/trunk/cmake/config-ix.cmake
53

@martell Could you please submit this change as a separate review? Doing so will allow this patch to land immediately.

martell added inline comments.May 25 2017, 3:19 PM
libcxx/trunk/cmake/config-ix.cmake
53

I'll just commit this now without that line and I will create another diff for that.

EricWF edited edge metadata.May 25 2017, 3:26 PM

@martell How close are we to getting the library linking on MinGW? Even with D33082 I can't get the libraries linking.

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

LIBCXXABI_BUILTINS_LIBRARY isn't defined by libc++ and we can't count on it being available here. I think you need to duplicate the logic from libc++abi into libc++.

53

SGTM. Thanks for working on this.

This revision was automatically updated to reflect the committed changes.

@martell How close are we to getting the library linking on MinGW? Even with D33082 I can't get the libraries linking.

We should be very close.
I can't fully comment on ld but I actually had this bootstrapping with lld before the new year.
Then I was able to rebuild clang with that bootstrapped toolchain. (without exception handling)

There seems to be some regression with linking however since 4.0 which I will work through with pcc and rui soon.
I'm currently suspecting https://reviews.llvm.org/rL289776 is causing __imp_ prefixing that mingw doesn't like.
I think it is due to differences in how mingw handles dllimport vs msvc.
@compnerd would probably be the one to know the answer to this.

@EricWF I think basic usage should be fine as soon as libc++abi gets linked properly. I suppose it haven't changed much since https://reviews.llvm.org/P7981
Back then replacing libc++abi with libsupc++ with few more hacks created almost-working libc++.

Those 2 bugs were blockers: http://lists.llvm.org/pipermail/cfe-dev/2017-April/053530.html

martell added inline comments.May 25 2017, 4:58 PM
libcxx/trunk/cmake/config-ix.cmake
45

The older versions of the patch didn't have LIBCXXABI
I went to cherry pick the commit onto master to push and got an older version I never meant to.
So I manually re added the changes. Thanks for pushing the fix for this.