[libcxx] [test] Fix filesystem_test_helper.h to compile for windows

[mstorsjo]

Summary

Use .string() instead of .native() in places where we want to combine paths with std::string.

Convert some methods to take a fs::path as parameter instead of std::string, for cases where they are called with paths as parameters (which can't be implicitly converted to std::string if the path's string_type is wstring).

[ldionne]

libcxx/test/support/filesystem_test_helper.h

27

I'm tempted to move all of those to

#if __has_include(<foo>)
#  include <foo>
#endif

With appropriate comments about what we're getting from those headers, it seems like this is easier to follow than complicated platform-specific #ifs.

32

Instead, I would create a layer of indirection for the utilities we're using here like this:

namespace utils {

#if defined(_WIN32)
  inline std::string getcwd() { /* windows impl */ }
  inline int mkdir(char const*, int) { /* windows impl */ }
  // etc
#else
  inline std::string getcwd() { /* posix impl */ }
  inline int mkdir(char const*, int) { /* posix impl */ }
  // etc
#endif

  inline bool exists(std::string const& path) { /* common impl */ }
} // end namespace utils

It's more verbose, but easier to follow and the code below can make unconditional calls to these functions without being cluttered by #ifs.

271

What are we missing to handle those correctly? I would rather not #ifdef them out if it's just "until we've fixed it". Or, at least, please put up another patch that removes them and mark it as a child of this one.

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

271

As mentioned in the post on libcxx-dev (which I admittedly didn't refer to in this review), symlinks on windows are pretty icky.

You either need to run your process with "elevated privileges", or be running your machine on windows 10 with developer mode enabled and do the syscall for creating the symlink with the flag SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE set. Not sure if that developer mode flag is set on e.g. buildbots and similar. But I guess we could and should try to use that - MS STL uses that at least.

And symbolic links in general on windows has a bit different semantics than on posix, iirc.

So far I've postponed looking into the whole symlinks bit (and ifdeffed out anything touching symlinks in the rest of the tests), but I guess I could at least try to create them here, to get this patch into a more acceptable form.

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

27

There are matching sets of ifdefs further down in this file, around the create_socket function, and within the tests in fs.op.funcs/fs.op.status/status.pass.cpp and fs.op.funcs/fs.op.symlink_status/symlink_status.pass.cpp as well. But maybe with __has_include() here, and a #define HAS_SOCKET which is used elsewhere?

Also, I'm running the tests against MS STL with clang-cl, but is the test suite supposed to work with MSVC itself as well? I presume that doesn't support __has_include().

32

Hmm, I can agree with the clarity, but for these functions, there's no #ifs anywhere in the calling code. Especially the duplication for getcwd feels a bit harsh, when the difference is just between ::getcwd and ::_getcwd.

I thought we could get rid of a few of these (that just add the underscore prefix) by defining _CRT_NONSTDC_NO_WARNINGS though, leaving just mkdir and ftruncate. But that ends up requiring manually linking against oldnames.lib (which normally is linked implicitly, but not in the setup things are done in the libc++ test suite)...

[mstorsjo]

Applied some amount of the suggested changes.

[mstorsjo]

Minor further touch-up (taking fs::path instead of std::string in more functions, using the is_dir parameter to create_symlink in one place), split out the socket specific bits to D89673.

[ldionne]

libcxx/test/support/filesystem_test_helper.h

27

Any support for MSVC is accidental at this point: I believe there was some amount of work done to make it work a few years back, however that is not tested on a regular basis and nobody is actively maintaining it.

32

I think we should link against it manually if that's what's done automatically normally. That makes us closer to running the compiler the way it's run by default.

59

You could even use using ::mkdir here!

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

32

Ok, will have a poke into doing that. The thing is that normally, when using the compiler normally, we'd be calling clang-cl, which automatically adds both the C runtime and oldnames.lib, but when invoking plain clang++.exe, it sidesteps all of that, but the clang driver only adds the C runtime. I guess the proper fix there would be to make the clang driver add oldnames.lib correspondingly there.

[mstorsjo]

Updated to using ::mkdir; for posix functions that are used as-is without any modification. Using fileno and chdir as is from windows functions is pending autolinking of oldnames.lib in the clang driver (or in the test suite), but updating this patch to current status quo here now.

[mstorsjo]

Redirect fewer functions, by enabling -D_CRT_NONSTDC_NO_WARNINGS and linking to oldnames.lib with D89702.

[ldionne]

Even better, thanks.

libcxx/test/support/filesystem_test_helper.h

 #ifndef FILESYSTEM_TEST_HELPER_H
 #define FILESYSTEM_TEST_HELPER_H
 
 #include "filesystem_include.h"
 
 #include <sys/stat.h> // for stat, mkdir, mkfifo
+#ifndef _WIN32
 #include <unistd.h> // for ftruncate, link, symlink, getcwd, chdir
+#else
+#include <io.h>
+#include <direct.h>
+#include <windows.h> // for CreateSymbolicLink, CreateHardLink
+#endif
 
 #include <cassert>
 #include <cstdio> // for printf
 #include <string>
 #include <chrono>
 #include <vector>
 #include <regex>
 
 #include "test_macros.h"
 #include "rapid-cxx-test.h"
 #include "format_string.h"
 
 // For creating socket files
 #if !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(_WIN32)
 # include <sys/socket.h>
 # include <sys/un.h>
 #endif
 
 namespace utils {
+#ifdef _WIN32
+    inline int mkdir(const char* path, int mode) { (void)mode; return ::_mkdir(path); }
+    inline int ftruncate(int fd, off_t length) { return ::_chsize(fd, length); }
+    inline int symlink(const char* oldname, const char* newname, bool is_dir) {
+        DWORD flags = is_dir ? SYMBOLIC_LINK_FLAG_DIRECTORY : 0;
+        if (CreateSymbolicLinkA(newname, oldname,
+                                flags | SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE))
+          return 0;
+        if (GetLastError() != ERROR_INVALID_PARAMETER)
+          return 1;
+        return !CreateSymbolicLinkA(newname, oldname, flags);
+    }
+    inline int link(const char *oldname, const char* newname) {
+        return !CreateHardLinkA(newname, oldname, NULL);
+    }
+#else
+    using ::mkdir;
+    using ::ftruncate;
+    inline int symlink(const char* oldname, const char* newname, bool is_dir) { (void)is_dir; return ::symlink(oldname, newname); }
+    using ::link;
+#endif
+
     inline std::string getcwd() {
         // Assume that path lengths are not greater than this.
         // This should be fine for testing purposes.
         char buf[4096];
         char* ret = ::getcwd(buf, sizeof(buf));
         assert(ret && "getcwd failed");
         return std::string(ret);
     }
 
     inline bool exists(std::string const& path) {
         struct ::stat tmp;
         return ::stat(path.c_str(), &tmp) == 0;
     }
 } // end namespace utils
 
 struct scoped_test_env
 {
     scoped_test_env() : test_root(available_cwd_path()) {
-        std::string cmd = "mkdir -p " + test_root.native();
+#ifdef _WIN32
+        // Windows mkdir can create multiple recursive directories
+        // if needed.
+        std::string cmd = "mkdir " + test_root.string();
+#else
+        std::string cmd = "mkdir -p " + test_root.string();
+#endif
         int ret = std::system(cmd.c_str());
         assert(ret == 0);
 
         // Ensure that the root_path is fully resolved, i.e. it contains no
         // symlinks. The filesystem tests depend on that. We do this after
         // creating the root_path, because `fs::canonical` requires the
         // path to exist.
         test_root = fs::canonical(test_root);
     }
 
     ~scoped_test_env() {
-        std::string cmd = "chmod -R 777 " + test_root.native();
+#ifdef _WIN32
+        std::string cmd = "rmdir /s /q " + test_root.string();
+        int ret;
+#else
+        std::string cmd = "chmod -R 777 " + test_root.string();
         int ret = std::system(cmd.c_str());
         assert(ret == 0);
 
-        cmd = "rm -r " + test_root.native();
+        cmd = "rm -r " + test_root.string();
+#endif
         ret = std::system(cmd.c_str());
         assert(ret == 0);
     }
 
     scoped_test_env(scoped_test_env const &) = delete;
     scoped_test_env & operator=(scoped_test_env const &) = delete;
 
     fs::path make_env_path(std::string p) { return sanitize_path(p); }
 
     std::string sanitize_path(std::string raw) {
         assert(raw.find("..") == std::string::npos);
-        std::string const& root = test_root.native();
+        std::string root = test_root.string();
         if (root.compare(0, root.size(), raw, 0, root.size()) != 0) {
             assert(raw.front() != '\\');
             fs::path tmp(test_root);
             tmp /= raw;
-            return std::move(const_cast<std::string&>(tmp.native()));
+            return tmp.string();
         }
         return raw;
     }
 
     // Purposefully using a size potentially larger than off_t here so we can
     // test the behavior of libc++fs when it is built with _FILE_OFFSET_BITS=64
     // but the caller is not (std::filesystem also uses uintmax_t rather than
     // off_t). On a 32-bit system this allows us to create a file larger than
     // 2GB.
-    std::string create_file(std::string filename, uintmax_t size = 0) {
-#if defined(__LP64__)
+    std::string create_file(fs::path filename_path, uintmax_t size = 0) {
+        std::string filename = filename_path.string();
+#if defined(__LP64__) || defined(_WIN32)
         auto large_file_fopen = fopen;
-        auto large_file_ftruncate = ftruncate;
+        auto large_file_ftruncate = utils::ftruncate;
         using large_file_offset_t = off_t;
 #else
         auto large_file_fopen = fopen64;
         auto large_file_ftruncate = ftruncate64;
         using large_file_offset_t = off64_t;
 #endif
 
         filename = sanitize_path(std::move(filename));
 
         if (size > std::numeric_limits<large_file_offset_t>::max()) {
             fprintf(stderr, "create_file(%s, %ju) too large\n",
                     filename.c_str(), size);
             abort();
         }
 
-        FILE* file = large_file_fopen(filename.c_str(), "we");
+#ifndef _WIN32
+#define FOPEN_CLOEXEC_FLAG "e"
+#else
+#define FOPEN_CLOEXEC_FLAG ""
+#endif
+        FILE* file = large_file_fopen(filename.c_str(), "w" FOPEN_CLOEXEC_FLAG);
         if (file == nullptr) {
             fprintf(stderr, "fopen %s failed: %s\n", filename.c_str(),
                     strerror(errno));
             abort();
         }
 
         if (large_file_ftruncate(
                 fileno(file), static_cast<large_file_offset_t>(size)) == -1) {
             fprintf(stderr, "ftruncate %s %ju failed: %s\n", filename.c_str(),
                     size, strerror(errno));
             fclose(file);
             abort();
         }
 
         fclose(file);
         return filename;
     }
 
-    std::string create_dir(std::string filename) {
+    std::string create_dir(fs::path filename_path) {
+        std::string filename = filename_path.string();
         filename = sanitize_path(std::move(filename));
-        int ret = ::mkdir(filename.c_str(), 0777); // rwxrwxrwx mode
+        int ret = utils::mkdir(filename.c_str(), 0777); // rwxrwxrwx mode
         assert(ret == 0);
         return filename;
     }
 
-    std::string create_symlink(std::string source,
-                               std::string to,
-                               bool sanitize_source = true) {
+    std::string create_symlink(fs::path source_path,
+                               fs::path to_path,
+                               bool sanitize_source = true,
+                               bool is_dir = false) {
+        std::string source = source_path.string();
+        std::string to = to_path.string();
         if (sanitize_source)
             source = sanitize_path(std::move(source));
         to = sanitize_path(std::move(to));
-        int ret = ::symlink(source.c_str(), to.c_str());
+        int ret = utils::symlink(source.c_str(), to.c_str(), is_dir);
         assert(ret == 0);
         return to;
     }
 
-    std::string create_hardlink(std::string source, std::string to) {
+    std::string create_hardlink(fs::path source_path, fs::path to_path) {
+        std::string source = source_path.string();
+        std::string to = to_path.string();
         source = sanitize_path(std::move(source));
         to = sanitize_path(std::move(to));
-        int ret = ::link(source.c_str(), to.c_str());
+        int ret = utils::link(source.c_str(), to.c_str());
         assert(ret == 0);
         return to;
     }
 
+#ifndef _WIN32
     std::string create_fifo(std::string file) {
         file = sanitize_path(std::move(file));
         int ret = ::mkfifo(file.c_str(), 0666); // rw-rw-rw- mode
         assert(ret == 0);
         return file;
     }
+#endif
 
   // Some platforms doesn't support socket files so we shouldn't even
   // allow tests to call this unguarded.
 #if !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(_WIN32)
     std::string create_socket(std::string file) {
         file = sanitize_path(std::move(file));
 
         ::sockaddr_un address;
private:
     // we generally don't use scoped_test_env from multiple threads, so
     // this is deemed acceptable.
     static inline fs::path available_cwd_path() {
         fs::path const cwd = utils::getcwd();
         fs::path const tmp = fs::temp_directory_path();
         fs::path const base = tmp / cwd.filename();
         int i = 0;
         fs::path p = base / ("static_env." + std::to_string(i));
-        while (utils::exists(p)) {
+        while (utils::exists(p.string())) {
             p = fs::path(base) / ("static_env." + std::to_string(++i));
         }
         return p;
     }
 };
 
 /// This class generates the following tree:
 ///

 ///     ├── non_empty_file
 ///     ├── symlink_to_dir -> dir1
 ///     └── symlink_to_empty_file -> empty_file
 ///
 class static_test_env {
     scoped_test_env env_;
 public:
     static_test_env() {
         env_.create_symlink("dne", "bad_symlink", false);
         env_.create_dir("dir1");
         env_.create_dir("dir1/dir2");
         env_.create_file("dir1/dir2/afile3");
         env_.create_dir("dir1/dir2/dir3");
         env_.create_file("dir1/dir2/dir3/file5");
         env_.create_file("dir1/dir2/file4");
-        env_.create_symlink("dir3", "dir1/dir2/symlink_to_dir3", false);
+        env_.create_symlink("dir3", "dir1/dir2/symlink_to_dir3", false, true);
         env_.create_file("dir1/file1");
         env_.create_file("dir1/file2", 42);
         env_.create_file("empty_file");
         env_.create_file("non_empty_file", 42);
         env_.create_symlink("dir1", "symlink_to_dir", false);
         env_.create_symlink("empty_file", "symlink_to_empty_file", false);
     }
 
std::size_t StrLen(CharT const* P) {
     return StrEnd(P) - P;
 }
 
 // Testing the allocation behavior of the code_cvt functions requires
 // *knowing* that the allocation was not done by "path::__str_".
 // This hack forces path to allocate enough memory.
 inline void PathReserve(fs::path& p, std::size_t N) {
   auto const& native_ref = p.native();
-  const_cast<std::string&>(native_ref).reserve(N);
+  const_cast<fs::path::string_type&>(native_ref).reserve(N);
 }
 
 template <class Iter1, class Iter2>
 bool checkCollectionsEqual(
     Iter1 start1, Iter1 const end1
   , Iter2 start2, Iter2 const end2
   )
 {
void check_libcxx_string(fs::filesystem_error const& Err) {
     std::string message = std::make_error_code(expected_err).message();
 
     std::string additional_msg = "";
     if (!opt_message.empty()) {
       additional_msg = opt_message + ": ";
     }
     auto transform_path = [](const fs::path& p) {
       if (p.native().empty())
-        return "\"\"";
-      return p.c_str();
+        return std::string("\"\"");
+      return p.string();
     };
     std::string format = [&]() -> std::string {
       switch (num_paths) {
       case 0:
         return format_string("filesystem error: in %s: %s%s", func_name,
                              additional_msg, message);
       case 1:
         return format_string("filesystem error: in %s: %s%s [%s]", func_name,
                              additional_msg, message,
-                             transform_path(expected_path1));
+                             transform_path(expected_path1).c_str());
       case 2:
         return format_string("filesystem error: in %s: %s%s [%s] [%s]",
                              func_name, additional_msg, message,
-                             transform_path(expected_path1),
-                             transform_path(expected_path2));
+                             transform_path(expected_path1).c_str(),
+                             transform_path(expected_path2).c_str());
       default:
         TEST_CHECK(false && "unexpected case");
         return "";
       }
     }();
     TEST_CHECK(format == Err.what());
     if (format != Err.what()) {
       fprintf(stderr,

libcxx/utils/libcxx/test/config.py

class Configuration(object):
     def configure_compile_flags(self):
         self.configure_default_compile_flags()
         # Configure extra flags
         compile_flags_str = self.get_lit_conf('compile_flags', '')
         self.cxx.compile_flags += shlex.split(compile_flags_str)
         if self.target_info.is_windows():
             # FIXME: Can we remove this?
             self.cxx.compile_flags += ['-D_CRT_SECURE_NO_WARNINGS']
+            # Don't warn about using common but nonstandard unprefixed functions
+            # like chdir, fileno.
+            self.cxx.compile_flags += ['-D_CRT_NONSTDC_NO_WARNINGS']
             # Required so that tests using min/max don't fail on Windows,
             # and so that those tests don't have to be changed to tolerate
             # this insanity.
             self.cxx.compile_flags += ['-DNOMINMAX']
         additional_flags = self.get_lit_conf('test_compiler_flags')
         if additional_flags:
             self.cxx.compile_flags += shlex.split(additional_flags)