[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Eric Fiselier via Phabricator]

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