Page MenuHomePhabricator

[libcxx] Build and test fixes for Windows
Needs RevisionPublic

Authored by hamzasood on Sep 10 2018, 10:03 AM.

Details

Summary

The patch fixes a few issues that arise when using libc++ on Windows.
Specifically:

  1. The CMake configuration added -Wall to the compilation, which actually means -Weverything in MSVC. This led to several thousand warnings per file.
  2. The experimental library was enabled by default. It doesn't really work (yet) on Windows, and in fact the build docs suggest to disable it, so I don't think that was a sensible default.
  3. There were some other errors that caused compilation errors in some of the tests.

I've identified a few other issues, but I'm not sure how best to fix them:

  1. support/win32/locale_win32.h includes xlocinfo.h, which ends up including yvals_core.h. That MSVC header defines feature test macros, among other things, that clash with macros defined in libc++. This is causing lots of test errors.
  2. <new> includes the ucrt header new.h in the hope that it'll declare get_new_handler etc. but new.h really just goes back and includes <new>. The end result is that nothing actually gets declared and so we're missing a few declarations, which causes lots of compilation errors.

Diff Detail

Event Timeline

hamzasood created this revision.Sep 10 2018, 10:03 AM
compnerd added inline comments.Sep 18 2018, 3:31 PM
include/filesystem
1396

This possibly changes the meaning on other targets. What was the error that this triggered?

test/support/test_macros.h
147

I think that the condition here is inverted, and should be enabled for MinGW32.

mclow.lists added inline comments.
test/support/verbose_assert.h
24

Why the renaming here?

What's the deal with sometimes using clog and sometimes cerr?
(I know, you didn't do this, but ???)

STL_MSFT added inline comments.
test/support/test_macros.h
147

What error prompted this? It hasn't caused problems when running libc++'s tests against MSVC's STL (with both C1XX and Clang).

hamzasood updated this revision to Diff 166124.Sep 19 2018, 7:24 AM

I've added a fix for another related issue: the tests would fail to link if libc++ is built only as a DLL.

The auto-linking doesn't look for a dynamic library unless _DLL is defined (which I think is done by passing /MD to CL), but the test runner doesn't use CL-style commands. This could either be solved by either manually defining that macro or just disabling auto-linking. The test runner already configures the linker correctly so disabling auto-linking seemed like the simplest option.

hamzasood added inline comments.Sep 19 2018, 7:25 AM
include/filesystem
1396

I've re-uploaded the patch with full context to make this clearer.

That's a member function on an exported type, which I don't think should have any visibility attributes. The specific issue in this case is that both the class type and the member function are marked as dllimport, which causes a compilation error.

test/support/test_macros.h
147

The comment above this says that it's copied from __config, but they must've gone out of sync at some point because __config doesn't have that extra section that I deleted.

This causes lots of errors when testing libc++. E.g. libc++ isn't declaring std::timespec on Windows because _LIBCPP_HAS_C11_FEATURES isn't defined, but the tests think that it's available so they try to use it (which causes compilation failures in lots of tests).

The better solution here might be to update __config to match this, but I'm not familiar with some of those platforms so this seemed like the safest approach for now.

147

This is a workaround for a libc++ issue rather than an MSVC one. See the response to @compnerd for the full details.

test/support/verbose_assert.h
24

Some of the casting that goes on in that template triggers an error under -fno-ms-extensions when given a function pointer (in this case: std::endl). Selecting between a standard/wide stream isn't needed for std::endl as it can go to either, so I've bypassed the selection by sending it straight to cerr.

The renaming is to clarify that an stderr stream (and nothing else) is being selected, which sort of justifies going directly to cerr.

mclow.lists added inline comments.Sep 19 2018, 10:06 AM
test/support/verbose_assert.h
26

The renaming is to clarify that an stderr stream is being selected.

And yet, there is clog, right there.

hamzasood added inline comments.Sep 19 2018, 11:18 AM
test/support/verbose_assert.h
26

I guess because that’s logging a programmer error (trying to print a type that isn’t streamable) whereas the others are test-related diagnostics? Not completely sure though.

EricWF requested changes to this revision.Oct 12 2018, 9:31 PM
EricWF added inline comments.
CMakeLists.txt
550

Couldn't we keep the "-Wall" here? And just add something else that adds "/W4"?

Also, we don't yet support building with MSVC's frontend. Only clang-cl. Does clang-cl not like this existing code?

include/filesystem
1396

Perhaps part of the problem here is that filesystem builds to a static library, not a shared one.

None the less, this macro is important once we move the definitions from libc++fs.a to libc++.so.

test/support/test_macros.h
147

What tests are failing?

utils/libcxx/test/config.py
518 ↗(On Diff #166124)

I think the correct fix here is to disabling linking libc++ when auto linking is enabled by the headers. Because we want to test that the auto link pragmas work.

This revision now requires changes to proceed.Oct 12 2018, 9:31 PM
hamzasood added inline comments.Oct 13 2018, 8:46 AM
CMakeLists.txt
550

The -Wall flag gets converted to -Weverything by Clang-CL; I don’t think we should keep it (it causes thousands of warnings per header).

/W4 is the equivalent flag on Windows.

include/filesystem
1396

I don’t think member functions are meant to have function visibility, and I wasn’t able to find any other such cases in libc++. That’ll always be a hard error on Windows because a dllimport class can’t have dllimport member functions.

test/support/test_macros.h
147

Any test that requires TEST_HAS_C11_FEATURES or TEST_HAS_TIMESPEC_GET. Those test macros aren’t in sync with the corresponding __config ones.

utils/libcxx/test/config.py
518 ↗(On Diff #166124)

In that case, it might be worth adding separate auto-linking tests that specifically use Clang-CL. Making it work here would involve defining _DLL for dynamic builds, but I’m not sure if that would interfere with the CRT.

Libc++ fs works on Windows now?