[11/N] [libcxx] Hook up a number of operation functions to their windows counterparts

[mstorsjo]

Summary

Use the corresponding wchar functions, named "_wfunc" instead of "func", where feasible, or reimplement functions with native windows APIs.

[amccarth]

I have a couple questions to consider inline. If those have already been thought through, then LTGM.

libcxx/src/filesystem/operations.cpp

401

Technically, there's nothing wrong here. It just _feels_ a little sketchy to call GetLastError() twice in a row.

422

Why not implement this with ::DeleteFile()? Does it have to do with waiting until the last handle is closed?

444

I'm mildly concerned about the way files are sometimes manipulated by HANDLE and sometimes by file descriptor.

If fd represents a file in text mode, then the caller needs to be very careful to understand that length must be the _byte_ offset, not the _char_ offset into the file.

456

Consider also MOVEFILE_WRITE_THROUGH for the case that it is indeed copied and then (probably) deleted.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

401

Yeah, it's not ideal, but then again, making a separate version of the set_errno() function that takes an error code as input, just for this case, also feels a bit superfluous (as the first call to GetLastError() shouldn't clobber the first one). Or would a default parameter, like set_errno(int err = GetLastError()) work?

422

Haven't considered/tested that, I can try that and run it over the testcases to see if it works.

The reason I ended up with this, was that I first just called _wremove(), followed by a _wrmdir() if the first failed and the path is a directory - as that was the most obvious path forward when porting over from the posix remove() function. That worked mostly, but failed in some symlink cases, and then I peeked at how the MS STL (which has the same license as libcxx, fwiw) implemented it, and I did the same.

444

File descriptors here are only an internal detail, fwiw, they're only used within the __copy_file function. But good catch about text vs binary mode; I guess the calls to FileDescriptor::create_with_status need to be amended with O_BINARY.

456

Sounds like a good thing to have, I can add that. These flags were based on what the MS STL use, and I didn't see that flag used there though.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

401

Changed set_errno() to take the error code as a parameter, with the parameter set by default to GetLastError().

422

I tried with this implementation, but this still fails on removing symlinks to directories:

int remove(const wchar_t *path) {
  if (DeleteFileW(path))
    return 0;
  int e = GetLastError();
  if (e == ERROR_ACCESS_DENIED) {
    if (is_directory(path)) {
      if (!RemoveDirectoryW(path)) 
        return set_errno();
      return 0;
    }
  }
  return set_errno(e);
}

444

Adding O_BINARY at the callsites that open file descriptors.

456

Adding that flag as it sounds beneficial, despite MS STL not using it.

[mstorsjo]

Made symlink_file_dir pass the inspected error code to set_errno(), passing O_BINARY to the open() function, passing the MOVEFILE_WRITE_THROUGH flag to MoveFileExW.

[amccarth]

LGTM, now.

[mstorsjo]

Rebased, already formally approved.

[mstorsjo]

Rebased, moved helpers to posix_compat.h.

[mstorsjo]

Retrigger CI.

[ldionne]

The integration between libc++ and the Windows shim looks great. I didn't look at the actual implementations using Windows APIs (I don't know those APIs and I trust @amccarth with that). LGTM once CI passes (can you please rebase to trigger, I'll add the LLVM monorepo to the diff now).

[mstorsjo]

Rebased, rerunning CI before pushing

libcxx/src/filesystem/operations.cpp

 namespace {
 
 using value_type = path::value_type;
 using string_type = path::string_type;
 
 struct FileDescriptor {
   const path& name;
   int fd = -1;
   StatT m_stat;
   file_status m_status;
 
   template <class... Args>
   static FileDescriptor create(const path* p, error_code& ec, Args... args) {
     ec.clear();
     int fd;
-    if ((fd = ::open(p->c_str(), args...)) == -1) {
+    if ((fd = detail::open(p->c_str(), args...)) == -1) {
       ec = capture_errno();
       return FileDescriptor{p};
     }
     return FileDescriptor(p, fd);
   }
 
   template <class... Args>
   static FileDescriptor create_with_status(const path* p, error_code& ec,
                                            Args... args) {
     FileDescriptor fd = create(p, ec, args...);
     if (!ec)
       fd.refresh_status(ec);
 
     return fd;
   }
 
   file_status get_status() const { return m_status; }
   StatT const& get_stat() const { return m_stat; }
 
   bool status_known() const { return _VSTD_FS::status_known(m_status); }
 
   file_status refresh_status(error_code& ec);
 
   void close() noexcept {
     if (fd != -1)
-      ::close(fd);
+      detail::close(fd);
     fd = -1;
   }
 
   FileDescriptor(FileDescriptor&& other)
       : name(other.name), fd(other.fd), m_stat(other.m_stat),
         m_status(other.m_status) {
     other.fd = -1;
     other.m_status = file_status{};
   }
 
   ~FileDescriptor() { close(); }
 
   FileDescriptor(FileDescriptor const&) = delete;
   FileDescriptor& operator=(FileDescriptor const&) = delete;
 
 private:
   explicit FileDescriptor(const path* p, int fd = -1) : name(*p), fd(fd) {}
 };
 
 perms posix_get_perms(const StatT& st) noexcept {
   return static_cast<perms>(st.st_mode) & perms::mask;
 }
 
 ::mode_t posix_convert_perms(perms prms) {
   return static_cast< ::mode_t>(prms & perms::mask);
 }
 
 file_status create_file_status(error_code& m_ec, path const& p,
                                const StatT& path_stat, error_code* ec) {
   if (ec)

 
 file_status posix_lstat(path const& p, error_code* ec) {
   StatT path_stat;
   return posix_lstat(p, path_stat, ec);
 }
 
 // http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html
 bool posix_ftruncate(const FileDescriptor& fd, off_t to_size, error_code& ec) {
-  if (::ftruncate(fd.fd, to_size) == -1) {
+  if (detail::ftruncate(fd.fd, to_size) == -1) {
     ec = capture_errno();
     return true;
   }
   ec.clear();
   return false;
 }
 
 bool posix_fchmod(const FileDescriptor& fd, const StatT& st, error_code& ec) {

 } // end namespace detail
 
 bool __copy_file(const path& from, const path& to, copy_options options,
                  error_code* ec) {
   using detail::FileDescriptor;
   ErrorHandler<bool> err("copy_file", ec, &to, &from);
 
   error_code m_ec;
-  FileDescriptor from_fd =
-      FileDescriptor::create_with_status(&from, m_ec, O_RDONLY | O_NONBLOCK);
+  FileDescriptor from_fd = FileDescriptor::create_with_status(
+      &from, m_ec, O_RDONLY | O_NONBLOCK | O_BINARY);
   if (m_ec)
     return err.report(m_ec);
 
   auto from_st = from_fd.get_status();
   StatT const& from_stat = from_fd.get_stat();
   if (!is_regular_file(from_st)) {
     if (not m_ec)
       m_ec = make_error_code(errc::not_supported);
if (!to_exists || overwrite_existing)
       return true;
     return err.report(errc::file_exists);
   }();
   if (!ShouldCopy)
     return false;
 
   // Don't truncate right away. We may not be opening the file we originally
   // looked at; we'll check this later.
-  int to_open_flags = O_WRONLY;
+  int to_open_flags = O_WRONLY | O_BINARY;
   if (!to_exists)
     to_open_flags |= O_CREAT;
   FileDescriptor to_fd = FileDescriptor::create_with_status(
       &to, m_ec, to_open_flags, from_stat.st_mode);
   if (m_ec)
     return err.report(m_ec);
 
   if (to_exists) {

 }
 
 void __copy_symlink(const path& existing_symlink, const path& new_symlink,
                     error_code* ec) {
   const path real_path(__read_symlink(existing_symlink, ec));
   if (ec && *ec) {
     return;
   }
-  // NOTE: proposal says you should detect if you should call
-  // create_symlink or create_directory_symlink. I don't think this
-  // is needed with POSIX
+#if defined(_LIBCPP_WIN32API)
+  error_code local_ec;
+  if (is_directory(real_path, local_ec))
+    __create_directory_symlink(real_path, new_symlink, ec);
+  else
+#endif
     __create_symlink(real_path, new_symlink, ec);
 }
 
 bool __create_directories(const path& p, error_code* ec) {
   ErrorHandler<bool> err("create_directories", ec, &p);
 
   error_code m_ec;
   auto const st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
if (!parent.empty()) {
     }
   }
   return __create_directory(p, ec);
 }
 
 bool __create_directory(const path& p, error_code* ec) {
   ErrorHandler<bool> err("create_directory", ec, &p);
 
-  if (::mkdir(p.c_str(), static_cast<int>(perms::all)) == 0)
+  if (detail::mkdir(p.c_str(), static_cast<int>(perms::all)) == 0)
     return true;
 
   if (errno == EEXIST) {
     error_code mec = capture_errno();
     error_code ignored_ec;
     const file_status st = status(p, ignored_ec);
     if (!is_directory(st)) {
       err.report(mec);
bool __create_directory(path const& p, path const& attributes, error_code* ec) {
   error_code mec;
   auto st = detail::posix_stat(attributes, attr_stat, &mec);
   if (!status_known(st))
     return err.report(mec);
   if (!is_directory(st))
     return err.report(errc::not_a_directory,
                       "the specified attribute path is invalid");
 
-  if (::mkdir(p.c_str(), attr_stat.st_mode) == 0)
+  if (detail::mkdir(p.c_str(), attr_stat.st_mode) == 0)
     return true;
 
   if (errno == EEXIST) {
     error_code mec = capture_errno();
     error_code ignored_ec;
     const file_status st = status(p, ignored_ec);
     if (!is_directory(st)) {
       err.report(mec);
     }
   } else {
     err.report(capture_errno());
   }
   return false;
 }
 
 void __create_directory_symlink(path const& from, path const& to,
                                 error_code* ec) {
   ErrorHandler<void> err("create_directory_symlink", ec, &from, &to);
-  if (::symlink(from.c_str(), to.c_str()) != 0)
+  if (detail::symlink_dir(from.c_str(), to.c_str()) == -1)
     return err.report(capture_errno());
 }
 
 void __create_hard_link(const path& from, const path& to, error_code* ec) {
   ErrorHandler<void> err("create_hard_link", ec, &from, &to);
-  if (::link(from.c_str(), to.c_str()) == -1)
+  if (detail::link(from.c_str(), to.c_str()) == -1)
     return err.report(capture_errno());
 }
 
 void __create_symlink(path const& from, path const& to, error_code* ec) {
   ErrorHandler<void> err("create_symlink", ec, &from, &to);
-  if (::symlink(from.c_str(), to.c_str()) == -1)
+  if (detail::symlink_file(from.c_str(), to.c_str()) == -1)
     return err.report(capture_errno());
 }
 
 path __current_path(error_code* ec) {
   ErrorHandler<path> err("current_path", ec);
 
   auto size = ::pathconf(".", _PC_PATH_MAX);
   _LIBCPP_ASSERT(size >= 0, "pathconf returned a 0 as max size");
 
   auto buff = unique_ptr<char[]>(new char[size + 1]);
   char* ret;
   if ((ret = ::getcwd(buff.get(), static_cast<size_t>(size))) == nullptr)
     return err.report(capture_errno(), "call to getcwd failed");
 
   return {buff.get()};
 }
 
 void __current_path(const path& p, error_code* ec) {
   ErrorHandler<void> err("current_path", ec, &p);
-  if (::chdir(p.c_str()) == -1)
+  if (detail::chdir(p.c_str()) == -1)
     err.report(capture_errno());
 }
 
 bool __equivalent(const path& p1, const path& p2, error_code* ec) {
   ErrorHandler<bool> err("equivalent", ec, &p1, &p2);
 
   error_code ec1, ec2;
   StatT st1 = {}, st2 = {};
#endif
   if (static_cast<size_t>(ret) >= size)
     return err.report(errc::value_too_large);
   buff[ret] = 0;
   return {buff.get()};
 }
 
 bool __remove(const path& p, error_code* ec) {
   ErrorHandler<bool> err("remove", ec, &p);
-  if (::remove(p.c_str()) == -1) {
+  if (detail::remove(p.c_str()) == -1) {
     if (errno != ENOENT)
       err.report(capture_errno());
     return false;
   }
   return true;
 }
 
 namespace {
if (mec == errc::no_such_file_or_directory)
       return 0;
     return err.report(mec);
   }
   return count;
 }
 
 void __rename(const path& from, const path& to, error_code* ec) {
   ErrorHandler<void> err("rename", ec, &from, &to);
-  if (::rename(from.c_str(), to.c_str()) == -1)
+  if (detail::rename(from.c_str(), to.c_str()) == -1)
     err.report(capture_errno());
 }
 
 void __resize_file(const path& p, uintmax_t size, error_code* ec) {
   ErrorHandler<void> err("resize_file", ec, &p);
-  if (::truncate(p.c_str(), static_cast< ::off_t>(size)) == -1)
+  if (detail::truncate(p.c_str(), static_cast< ::off_t>(size)) == -1)
     return err.report(capture_errno());
 }
 
 space_info __space(const path& p, error_code* ec) {
   ErrorHandler<void> err("space", ec, &p);
   space_info si;
   struct statvfs m_svfs = {};
   if (::statvfs(p.c_str(), &m_svfs) == -1) {

libcxx/src/filesystem/posix_compat.h

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 //
 // POSIX-like portability helper functions.
 //
 // These generally behave like the proper posix functions, with these
 // exceptions:
 // On Windows, they take paths in wchar_t* form, instead of char* form.
+// The symlink() function is split into two frontends, symlink_file()
+// and symlink_dir().
 //
 // These are provided within an anonymous namespace within the detail
 // namespace - callers need to include this header and call them as
 // detail::function(), regardless of platform.
 //
 
 #ifndef POSIX_COMPAT_H
 #define POSIX_COMPAT_H

 #define S_ISDIR(m)      (((m) & _S_IFMT) == _S_IFDIR)
 #define S_ISCHR(m)      (((m) & _S_IFMT) == _S_IFCHR)
 #define S_ISFIFO(m)     (((m) & _S_IFMT) == _S_IFIFO)
 #define S_ISREG(m)      (((m) & _S_IFMT) == _S_IFREG)
 #define S_ISBLK(m)      (((m) & _S_IFMT) == _S_IFBLK)
 #define S_ISLNK(m)      (((m) & _S_IFMT) == _S_IFLNK)
 #define S_ISSOCK(m)     (((m) & _S_IFMT) == _S_IFSOCK)
 
+#define O_NONBLOCK 0
+
 
 // There were 369 years and 89 leap days from the Windows epoch
 // (1601) to the Unix epoch (1970).
 #define FILE_TIME_OFFSET_SECS (uint64_t(369 * 365 + 89) * (24 * 60 * 60))
 
 TimeSpec filetime_to_timespec(LARGE_INTEGER li) {
   TimeSpec ret;
   ret.tv_sec = li.QuadPart / 10000000 - FILE_TIME_OFFSET_SECS;

 int lstat(const wchar_t *path, StatT *buf) {
   return stat_file(path, buf, FILE_FLAG_OPEN_REPARSE_POINT);
 }
 
 int fstat(int fd, StatT *buf) {
   HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
   return stat_handle(h, buf);
 }
+
+int mkdir(const wchar_t *path, int permissions) {
+  (void)permissions;
+  return _wmkdir(path);
+}
+
+int symlink_file_dir(const wchar_t *oldname, const wchar_t *newname,
+                     bool is_dir) {
+  DWORD flags = is_dir ? SYMBOLIC_LINK_FLAG_DIRECTORY : 0;
+  if (CreateSymbolicLinkW(newname, oldname,
+                          flags | SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE))
+    return 0;
+  int e = GetLastError();
+  if (e != ERROR_INVALID_PARAMETER)
+    return set_errno(e);
+  if (CreateSymbolicLinkW(newname, oldname, flags))
+    return 0;
+  return set_errno();
+}
+
+int symlink_file(const wchar_t *oldname, const wchar_t *newname) {
+  return symlink_file_dir(oldname, newname, false);
+}
+
+int symlink_dir(const wchar_t *oldname, const wchar_t *newname) {
+  return symlink_file_dir(oldname, newname, true);
+}
+
+int link(const wchar_t *oldname, const wchar_t *newname) {
+  if (CreateHardLinkW(newname, oldname, nullptr))
+    return 0;
+  return set_errno();
+}
+
+int remove(const wchar_t *path) {
+  detail::WinHandle h(path, DELETE, FILE_FLAG_OPEN_REPARSE_POINT);
+  if (!h)
+    return set_errno();
+  FILE_DISPOSITION_INFO info;
+  info.DeleteFile = TRUE;
+  if (!SetFileInformationByHandle(h, FileDispositionInfo, &info, sizeof(info)))
+    return set_errno();
+  return 0;
+}
+
+int truncate_handle(HANDLE h, off_t length) {
+  LARGE_INTEGER size_param;
+  size_param.QuadPart = length;
+  if (!SetFilePointerEx(h, size_param, 0, FILE_BEGIN))
+    return set_errno();
+  if (!SetEndOfFile(h))
+    return set_errno();
+  return 0;
+}
+
+int ftruncate(int fd, off_t length) {
+  HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  return truncate_handle(h, length);
+}
+
+int truncate(const wchar_t *path, off_t length) {
+  detail::WinHandle h(path, GENERIC_WRITE, 0);
+  if (!h)
+    return set_errno();
+  return truncate_handle(h, length);
+}
+
+int rename(const wchar_t *from, const wchar_t *to) {
+  if (!(MoveFileExW(from, to,
+                    MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING |
+                        MOVEFILE_WRITE_THROUGH)))
+    return set_errno();
+  return 0;
+}
+
+template <class... Args> int open(const wchar_t *filename, Args... args) {
+  return _wopen(filename, args...);
+}
+int close(int fd) { return _close(fd); }
+int chdir(const wchar_t *path) { return _wchdir(path); }
 #else
+int symlink_file(const char *oldname, const char *newname) {
+  return ::symlink(oldname, newname);
+}
+int symlink_dir(const char *oldname, const char *newname) {
+  return ::symlink(oldname, newname);
+}
+using ::chdir;
+using ::close;
 using ::fstat;
+using ::ftruncate;
+using ::link;
 using ::lstat;
+using ::mkdir;
+using ::open;
+using ::remove;
+using ::rename;
 using ::stat;
+using ::truncate;
+
+#define O_BINARY 0
+
 #endif
 
 } // namespace
 } // end namespace detail
 
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 
 #endif // POSIX_COMPAT_H