[PATCH] D51868: [libcxx] Build and test fixes for Windows
xbolva00 added a comment. Libc++ fs works on Windows now? https://reviews.llvm.org/D51868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
hamzasood added inline comments. Comment at: CMakeLists.txt:550 +endif() add_compile_flags_if_supported( +-Wextra -W -Wwrite-strings EricWF wrote: > 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? 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. Comment at: include/filesystem:1396 - _LIBCPP_FUNC_VIS void __create_what(int __num_paths); EricWF wrote: > hamzasood wrote: > > compnerd wrote: > > > This possibly changes the meaning on other targets. What was the error > > > that this triggered? > > 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. > 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. 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. Comment at: test/support/test_macros.h:147 -# elif defined(_WIN32) -#if defined(_MSC_VER) && !defined(__MINGW32__) -# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library EricWF wrote: > hamzasood wrote: > > hamzasood wrote: > > > STL_MSFT wrote: > > > > compnerd wrote: > > > > > I think that the condition here is inverted, and should be enabled > > > > > for MinGW32. > > > > What error prompted this? It hasn't caused problems when running > > > > libc++'s tests against MSVC's STL (with both C1XX and Clang). > > > 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. > > This is a workaround for a libc++ issue rather than an MSVC one. See the > > response to @compnerd for the full details. > > What tests are failing? 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. Comment at: utils/libcxx/test/config.py:518 self.cxx.compile_flags += ['-DNOMINMAX'] +# Disable auto-linking; the library is linked manually when +# configuring the linker flags. EricWF wrote: > 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. 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. https://reviews.llvm.org/D51868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
EricWF requested changes to this revision. EricWF added inline comments. This revision now requires changes to proceed. Comment at: CMakeLists.txt:550 +endif() add_compile_flags_if_supported( +-Wextra -W -Wwrite-strings 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? Comment at: include/filesystem:1396 - _LIBCPP_FUNC_VIS void __create_what(int __num_paths); hamzasood wrote: > compnerd wrote: > > This possibly changes the meaning on other targets. What was the error > > that this triggered? > 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. 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. Comment at: test/support/test_macros.h:147 -# elif defined(_WIN32) -#if defined(_MSC_VER) && !defined(__MINGW32__) -# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library hamzasood wrote: > hamzasood wrote: > > STL_MSFT wrote: > > > compnerd wrote: > > > > I think that the condition here is inverted, and should be enabled for > > > > MinGW32. > > > What error prompted this? It hasn't caused problems when running libc++'s > > > tests against MSVC's STL (with both C1XX and Clang). > > 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. > This is a workaround for a libc++ issue rather than an MSVC one. See the > response to @compnerd for the full details. What tests are failing? Comment at: utils/libcxx/test/config.py:518 self.cxx.compile_flags += ['-DNOMINMAX'] +# Disable auto-linking; the library is linked manually when +# configuring the linker flags. 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. https://reviews.llvm.org/D51868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
hamzasood added inline comments.
Comment at: test/support/verbose_assert.h:26
static_assert(ST == -1, "specialization required for ST != -1");
static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
};
mclow.lists wrote:
> > The renaming is to clarify that an `stderr` stream is being selected.
> And yet, there is `clog`, right there.
>
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.
https://reviews.llvm.org/D51868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
mclow.lists added inline comments.
Comment at: test/support/verbose_assert.h:26
static_assert(ST == -1, "specialization required for ST != -1");
static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
};
> The renaming is to clarify that an `stderr` stream is being selected.
And yet, there is `clog`, right there.
https://reviews.llvm.org/D51868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
hamzasood added inline comments.
Comment at: include/filesystem:1396
- _LIBCPP_FUNC_VIS
void __create_what(int __num_paths);
compnerd wrote:
> This possibly changes the meaning on other targets. What was the error that
> this triggered?
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.
Comment at: test/support/test_macros.h:147
-# elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
STL_MSFT wrote:
> compnerd wrote:
> > I think that the condition here is inverted, and should be enabled for
> > MinGW32.
> What error prompted this? It hasn't caused problems when running libc++'s
> tests against MSVC's STL (with both C1XX and Clang).
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.
Comment at: test/support/test_macros.h:147
-# elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
hamzasood wrote:
> STL_MSFT wrote:
> > compnerd wrote:
> > > I think that the condition here is inverted, and should be enabled for
> > > MinGW32.
> > What error prompted this? It hasn't caused problems when running libc++'s
> > tests against MSVC's STL (with both C1XX and Clang).
> 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.
This is a workaround for a libc++ issue rather than an MSVC one. See the
response to @compnerd for the full details.
Comment at: test/support/verbose_assert.h:24
: (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
static_assert(ST == -1, "specialization required for ST != -1");
mclow.lists wrote:
> Why the renaming here?
>
> What's the deal with sometimes using `clog` and sometimes `cerr`?
> (I know, you didn't do this, but ???)
>
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`.
https://reviews.llvm.org/D51868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
hamzasood updated this revision to Diff 166124.
hamzasood added a comment.
Herald added a subscriber: christof.
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.
https://reviews.llvm.org/D51868
Files:
CMakeLists.txt
include/filesystem
test/std/strings/c.strings/cuchar.pass.cpp
test/support/test_macros.h
test/support/verbose_assert.h
utils/libcxx/test/config.py
Index: utils/libcxx/test/config.py
===
--- utils/libcxx/test/config.py
+++ utils/libcxx/test/config.py
@@ -515,6 +515,10 @@
# and so that those tests don't have to be changed to tolerate
# this insanity.
self.cxx.compile_flags += ['-DNOMINMAX']
+# Disable auto-linking; the library is linked manually when
+# configuring the linker flags.
+if self.cxx_stdlib_under_test == 'libc++':
+self.cxx.compile_flags += ['-D_LIBCPP_NO_AUTO_LINK']
additional_flags = self.get_lit_conf('test_compiler_flags')
if additional_flags:
self.cxx.compile_flags += shlex.split(additional_flags)
Index: test/support/verbose_assert.h
===
--- test/support/verbose_assert.h
+++ test/support/verbose_assert.h
@@ -21,18 +21,18 @@
template ::value ? 1
: (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
static_assert(ST == -1, "specialization required for ST != -1");
static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
};
template
-struct SelectStream {
+struct SelectErrStream {
static void Print(Tp const& val) { std::cerr << val; }
};
template
-struct SelectStream {
+struct SelectErrStream {
static void Print(Tp const& val) { std::wcerr << val; }
};
@@ -86,14 +86,14 @@
template
friend LogType& operator<<(LogType& log, Tp const& value) {
if (!log.is_disabled) {
-SelectStream::Print(value);
+SelectErrStream::Print(value);
}
return log;
}
friend LogType& operator<<(LogType& log, EndLType* m) {
if (!log.is_disabled) {
-SelectStream::Print(m);
+std::cerr << m;
}
return log;
}
Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -143,11 +143,6 @@
# define TEST_HAS_C11_FEATURES
# define TEST_HAS_TIMESPEC_GET
#endif
-# elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
-# define TEST_HAS_TIMESPEC_GET
-#endif
# endif
#endif
Index: test/std/strings/c.strings/cuchar.pass.cpp
===
--- test/std/strings/c.strings/cuchar.pass.cpp
+++ test/std/strings/c.strings/cuchar.pass.cpp
@@ -8,6 +8,9 @@
//===--===//
//
// XFAIL: *
+//
+// The MSVC stdlib has cuchar, which causes this test to pass unexpectedly.
+// UNSUPPORTED: windows
//
Index: include/filesystem
===
--- include/filesystem
+++ include/filesystem
@@ -1390,7 +1390,6 @@
return __storage_->__what_.c_str();
}
- _LIBCPP_FUNC_VIS
void __create_what(int __num_paths);
private:
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -67,7 +67,7 @@
option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ON)
+option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" $)
set(ENABLE_FILESYSTEM_DEFAULT ${LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY})
if (WIN32)
set(ENABLE_FILESYSTEM_DEFAULT OFF)
@@ -542,8 +542,13 @@
# Warning flags ===
add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+if (MSVC)
+add_compile_flags(/W4)
+else()
+add_compile_flags_if_supported(-Wall)
+endif()
add_compile_flags_if_supported(
--Wall -Wextra -W -Wwrite-strings
+-Wextra -W -Wwrite-strings
-Wno-unused-parameter -Wno-long-long
[PATCH] D51868: [libcxx] Build and test fixes for Windows
STL_MSFT added inline comments. Comment at: test/support/test_macros.h:147 -# elif defined(_WIN32) -#if defined(_MSC_VER) && !defined(__MINGW32__) -# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library compnerd wrote: > I think that the condition here is inverted, and should be enabled for > MinGW32. What error prompted this? It hasn't caused problems when running libc++'s tests against MSVC's STL (with both C1XX and Clang). Repository: rCXX libc++ https://reviews.llvm.org/D51868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
mclow.lists added inline comments.
Comment at: test/support/verbose_assert.h:24
: (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
static_assert(ST == -1, "specialization required for ST != -1");
Why the renaming here?
What's the deal with sometimes using `clog` and sometimes `cerr`?
(I know, you didn't do this, but ???)
Repository:
rCXX libc++
https://reviews.llvm.org/D51868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
compnerd added inline comments. Herald added a subscriber: libcxx-commits. Comment at: include/filesystem:1396 - _LIBCPP_FUNC_VIS void __create_what(int __num_paths); This possibly changes the meaning on other targets. What was the error that this triggered? Comment at: test/support/test_macros.h:147 -# elif defined(_WIN32) -#if defined(_MSC_VER) && !defined(__MINGW32__) -# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library I think that the condition here is inverted, and should be enabled for MinGW32. Repository: rCXX libc++ https://reviews.llvm.org/D51868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51868: [libcxx] Build and test fixes for Windows
hamzasood created this revision.
hamzasood added reviewers: rnk, EricWF, compnerd, smeenai.
Herald added subscribers: cfe-commits, ldionne, mgorny.
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. `` 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 ``.
The end result is that nothing actually gets declared and so we're missing a
few declarations, which causes lots of compilation errors.
Repository:
rCXX libc++
https://reviews.llvm.org/D51868
Files:
CMakeLists.txt
include/filesystem
test/std/strings/c.strings/cuchar.pass.cpp
test/support/test_macros.h
test/support/verbose_assert.h
Index: test/support/verbose_assert.h
===
--- test/support/verbose_assert.h
+++ test/support/verbose_assert.h
@@ -21,18 +21,18 @@
template ::value ? 1
: (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
static_assert(ST == -1, "specialization required for ST != -1");
static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
};
template
-struct SelectStream {
+struct SelectErrStream {
static void Print(Tp const& val) { std::cerr << val; }
};
template
-struct SelectStream {
+struct SelectErrStream {
static void Print(Tp const& val) { std::wcerr << val; }
};
@@ -86,14 +86,14 @@
template
friend LogType& operator<<(LogType& log, Tp const& value) {
if (!log.is_disabled) {
-SelectStream::Print(value);
+SelectErrStream::Print(value);
}
return log;
}
friend LogType& operator<<(LogType& log, EndLType* m) {
if (!log.is_disabled) {
-SelectStream::Print(m);
+std::cerr << m;
}
return log;
}
Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -143,11 +143,6 @@
# define TEST_HAS_C11_FEATURES
# define TEST_HAS_TIMESPEC_GET
#endif
-# elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-# define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
-# define TEST_HAS_TIMESPEC_GET
-#endif
# endif
#endif
Index: test/std/strings/c.strings/cuchar.pass.cpp
===
--- test/std/strings/c.strings/cuchar.pass.cpp
+++ test/std/strings/c.strings/cuchar.pass.cpp
@@ -8,6 +8,9 @@
//===--===//
//
// XFAIL: *
+//
+// The MSVC stdlib has cuchar, which causes this test to pass unexpectedly.
+// UNSUPPORTED: windows
//
Index: include/filesystem
===
--- include/filesystem
+++ include/filesystem
@@ -1393,7 +1393,6 @@
return __storage_->__what_.c_str();
}
- _LIBCPP_FUNC_VIS
void __create_what(int __num_paths);
private:
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -67,7 +67,7 @@
option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ON)
+option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" $)
set(ENABLE_FILESYSTEM_DEFAULT ${LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY})
if (WIN32)
set(ENABLE_FILESYSTEM_DEFAULT OFF)
@@ -542,8 +542,13 @@
# Warning flags ===
add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+if (MSVC)
+add_compile_flags(/W4)
+else()
+add_compile_flags_if_supported(-Wall)
+endif()
add_compile_flags_if_supported(
--Wall -Wextra -W -Wwrite-strings
+-Wextra -W -Wwrite-strings
-Wno-unused-parameter -Wno-long-long
-Werror=return-type)
if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
