[8/N] [libcxx] Fix the preexisting directory_iterator code for windows

[mstorsjo]

Summary

The directory_iterator.cpp file did contain an incomplete, non-working implementation for windows.

Change it to use the wchar version of the APIs.

Don't set the windows specific errors from GetLastError() as code in the generic category; remap the errors to the std::errc values.

Error out cleanly on empty paths.

Invoke FindFirstFile on <directoryname>/* to actually list the entries of the directory.

If the first entry retured by FindFirstFile is to be skipped (e.g. being "." or ".."), call advance() (which calls FindNextFile and loops) which doesn't return until a valid entry is found (or the end is reached).

[rnk]

Hi, I'm pretty busy right now, and I'm falling behind on other code reviews. Can you send this libc++ patch series to @amccarth? He's on my team, and is familiar with the Win32 API, but is unfamiliar with mingw conventions. Hopefully he has enough experience to help implement these parts of the standard library.

[mstorsjo]

In D91140#2386807, @rnk wrote:

Hi, I'm pretty busy right now, and I'm falling behind on other code reviews. Can you send this libc++ patch series to @amccarth? He's on my team, and is familiar with the Win32 API, but is unfamiliar with mingw conventions. Hopefully he has enough experience to help implement these parts of the standard library.

Fair enough, thanks! Luckily, essentially nothing in this patchset is mingw specific (I've tried to steer away from anything that differs, that can be messy). Maybe the only real difference lies in being able to use the __int128 type, see D91139.

[amccarth]

This is pretty close.

libcxx/src/filesystem/directory_iterator.cpp

85–86

I know you didn't change this line, but it looks wrong to me.

MAXDWORD is a macro for 0xffffffff, which I believe the compiler will interpret as unsigned int (or maybe unsigned long, which is only 32 bits in 64-bit Windows). Adding 1 will cause it to wrap to 0. Multiplying by 0 effectively ignores the higher bits, so we end up with just the low 32 bits of the size, which will be extended to a uintmax_t upon return.

libcxx/src/filesystem/filesystem_common.h

23

It's a real shame we have to include <windows.h> in a header. Even with WIN32_LEAN_AND_MEAN and NOMINMAX, it really pollutes the macro and global namespaces.

Does code that includes <filesystem> transitively get this pulled into their code? If so, would it be possible to move the definitions to a .cpp file and only include <windows> there?

130

constexpr?

184

Needs inline for ODR, right?

191

inline

[mstorsjo]

Thanks for having a look!

libcxx/src/filesystem/directory_iterator.cpp

85–86

Good catch, I can try to fix that while fixing up the preexisting code here.

libcxx/src/filesystem/filesystem_common.h

23

No, luckily this header is only used by the implementation itself, it doesn't end up polluting user code. This header is essentially only used by src/filesystem/operations.cpp and src/filesystem/directory_iterator.cpp for stuff shared by both of them.

130

Will do

184

Hmm, I'm not entirely sure how this works, when this is an anonymous namespace (which essentially works as static in C afaik?), as that isn't supposed to be visible outside of the current translation unit (any similarity to similar named things in other translation units can be coincidental), so I'm not sure if such things can be made ODR... But this is the mechanism used for the header as it is right now.

If we can't get these shared between the two translation units in an anonymous namespace, I guess it'd be better to give at least the table a non-anonymous symbol so that it can be shared properly.

[mstorsjo]

libcxx/src/filesystem/filesystem_common.h

23

I removed the need for windows.h in the shared header anyway, without excessive jumping through hoops.

184

Moved the table to a non-anonymous namespace to make one single instanance that can be shared between the two translation units. I didn't add inline as it doesn't have any effect on ODR in an anonymous namespace, so it matches the existing functions in the header.

[mstorsjo]

Fixed the file size calculation by casting to uint64_t (and doing shifting instead of multiplying by MAXDWORD+1), moved the error mapping table to operations.cpp, removed inclusions of windows.h in the filesystem_common.h header.

[amccarth]

I'm not a libcxx owner, so I'm not sure what other hoops you should jump through for them, but this looks good to me. Thanks for the improvements!

[curdeius]

Minor comment only. It would be cool to have the CI triggered...

libcxx/src/filesystem/operations.cpp

328

You could use an inline variable instead of putting it into an anonymous namespace. Personally I find it cleaner.

[mstorsjo]

Moved the error mapping table into an inline variable to avoid one extra anonymous namespace.

[mstorsjo]

Try to re-trigger CI...

[mstorsjo]

Third time's the charm?

[ldionne]

libcxx/src/filesystem/directory_iterator.cpp

75

I think adding a comment here to signify that this is the Windows branch would add readability.

121

Isn't that a use-after-free? (root / "*").c_str() returns a pointer to the internal string held in the temporary object created by (root / "*"), unless I'm missing a subtlety here. I don't think the lifetime of objects in an expression used as a function argument is extended automatically.

libcxx/src/filesystem/filesystem_common.h

21

Please indent #includes in between.

[mstorsjo]

libcxx/src/filesystem/directory_iterator.cpp

75

Sure, I can add that.

121

I don't know the spec bits exactly that allow this, but I'm fairly sure it's supposed to work. An empirical test: https://godbolt.org/z/za8K46

I'm not too familiar with the C++ spec itself to reference it myself, but https://stackoverflow.com/a/584835/3115956 claims that "12.2 Temporary objects" in the C++ standard covers this, saying that the destructors for such temporaries aren't called until at the end of the full-expression.

libcxx/src/filesystem/filesystem_common.h

21

Ok, will do that for this block I'm touching here. There's more unindented blocks in the same file, but I'm keeping them untouched here.

[mstorsjo]

Indented includes in a new ifdef block, added a comment after an #else to improve readability.

[mstorsjo]

Reupload to readd the repo, to trigger CI

[mstorsjo]

Rebased, rerunning CI

[ldionne]

libcxx/src/filesystem/directory_iterator.cpp

121

Uh, you're right, I guess that's an enormous brain fart on my end. Like they say in the SO answer, expression templates are the obvious use case that make use of this property.

See http://eel.is/c++draft/class.temporary#4 for details.

libcxx/src/filesystem/directory_iterator.cpp

 //===------------------ directory_iterator.cpp ----------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "filesystem"
 #include "__config"
 #if defined(_LIBCPP_WIN32API)
 #define WIN32_LEAN_AND_MEAN
+#define NOMINMAX
 #include <windows.h>
 #else
 #include <dirent.h>
 #endif
 #include <errno.h>
 
 #include "filesystem_common.h"
 
static pair<string_view, file_type> posix_readdir(DIR* dir_stream,
   if ((dir_entry_ptr = ::readdir(dir_stream)) == nullptr) {
     if (errno)
       ec = capture_errno();
     return {};
   } else {
     return {dir_entry_ptr->d_name, get_file_type(dir_entry_ptr, 0)};
   }
 }
 #else
 
-static file_type get_file_type(const WIN32_FIND_DATA& data) {
-  //auto attrs = data.dwFileAttributes;
-  // FIXME(EricWF)
-  return file_type::unknown;
+static file_type get_file_type(const WIN32_FIND_DATAW& data) {
+  if (data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT &&
+      data.dwReserved0 == IO_REPARSE_TAG_SYMLINK)
+    return file_type::symlink;
+  if (data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+    return file_type::directory;
+  return file_type::regular;
 }
-static uintmax_t get_file_size(const WIN32_FIND_DATA& data) {
-  return (data.nFileSizeHigh * (MAXDWORD + 1)) + data.nFileSizeLow;
+static uintmax_t get_file_size(const WIN32_FIND_DATAW& data) {
+  return (static_cast<uint64_t>(data.nFileSizeHigh) << 32) + data.nFileSizeLow;
 }
-static file_time_type get_write_time(const WIN32_FIND_DATA& data) {
+static file_time_type get_write_time(const WIN32_FIND_DATAW& data) {
   ULARGE_INTEGER tmp;
   const FILETIME& time = data.ftLastWriteTime;
   tmp.u.LowPart = time.dwLowDateTime;
   tmp.u.HighPart = time.dwHighDateTime;
   return file_time_type(file_time_type::duration(tmp.QuadPart));
 }
 
 #endif
public:
   __dir_stream(__dir_stream&& __ds) noexcept : __stream_(__ds.__stream_),
                                                __root_(move(__ds.__root_)),
                                                __entry_(move(__ds.__entry_)) {
     __ds.__stream_ = INVALID_HANDLE_VALUE;
   }
 
   __dir_stream(const path& root, directory_options opts, error_code& ec)
       : __stream_(INVALID_HANDLE_VALUE), __root_(root) {
-    __stream_ = ::FindFirstFile(root.c_str(), &__data_);
+    if (root.native().empty()) {
+      ec = make_error_code(errc::no_such_file_or_directory);
+      return;
+    }
+    __stream_ = ::FindFirstFileW((root / "*").c_str(), &__data_);
     if (__stream_ == INVALID_HANDLE_VALUE) {
-      ec = error_code(::GetLastError(), generic_category());
+      ec = detail::make_windows_error(GetLastError());
       const bool ignore_permission_denied =
           bool(opts & directory_options::skip_permission_denied);
       if (ignore_permission_denied && ec.value() == ERROR_ACCESS_DENIED)
         ec.clear();
       return;
     }
+    if (!assign())
+      advance(ec);
   }
 
   ~__dir_stream() noexcept {
     if (__stream_ == INVALID_HANDLE_VALUE)
       return;
     close();
   }
 
   bool good() const noexcept { return __stream_ != INVALID_HANDLE_VALUE; }
 
   bool advance(error_code& ec) {
-    while (::FindNextFile(__stream_, &__data_)) {
-      if (!strcmp(__data_.cFileName, ".") || strcmp(__data_.cFileName, ".."))
-        continue;
-      // FIXME: Cache more of this
-      //directory_entry::__cached_data cdata;
-      //cdata.__type_ = get_file_type(__data_);
-      //cdata.__size_ = get_file_size(__data_);
-      //cdata.__write_time_ = get_write_time(__data_);
-      __entry_.__assign_iter_entry(
-          __root_ / __data_.cFileName,
-          directory_entry::__create_iter_result(detail::get_file_type(__data)));
-      return true;
-    }
-    ec = error_code(::GetLastError(), generic_category());
-    close();
-    return false;
-  }
+    while (::FindNextFileW(__stream_, &__data_)) {
+      if (assign())
+        return true;
+    }
+    close();
+    return false;
+  }
+
+  bool assign() {
+    if (!wcscmp(__data_.cFileName, L".") || !wcscmp(__data_.cFileName, L".."))
+      return false;
+        directory_entry::__create_iter_result(detail::get_file_type(__data_)));
 
 private:
   error_code close() noexcept {
     error_code ec;
     if (!::FindClose(__stream_))
-      ec = error_code(::GetLastError(), generic_category());
+      ec = detail::make_windows_error(GetLastError());
     __stream_ = INVALID_HANDLE_VALUE;
     return ec;
   }
 
   HANDLE __stream_{INVALID_HANDLE_VALUE};
-  WIN32_FIND_DATA __data_;
+  WIN32_FIND_DATAW __data_;
 
 public:
   path __root_;
   directory_entry __entry_;
 };
 #else
 class __dir_stream {
 public:

libcxx/src/filesystem/filesystem_common.h

 #include "__config"
 #include "filesystem"
 #include "array"
 #include "chrono"
 #include "climits"
 #include "cstdlib"
 #include "ctime"
 
+#if !defined(_LIBCPP_WIN32API)
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/statvfs.h>
 #include <sys/time.h> // for ::utimes as used in __last_write_time
 #include <fcntl.h>    /* values for fchmodat */
+#endif
 
 #include "../include/apple_availability.h"
 
 #if !defined(__APPLE__)
 // We can use the presence of UTIME_OMIT to detect platforms that provide
 // utimensat.
 #if defined(UTIME_OMIT)
 #define _LIBCPP_USE_UTIMENSAT

 #define PS(x) (L##x)
 #else
 #define PS(x) (x)
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 namespace detail {
+
+#if defined(_LIBCPP_WIN32API)
+// Non anonymous, to allow access from two translation units.
+errc __win_err_to_errc(int err);
+#endif
+
 namespace {
 
 static string format_string_imp(const char* msg, ...) {
   // we might need a second shot at this, so pre-emptivly make a copy
   struct GuardVAList {
     va_list& target;
     bool active = true;
     GuardVAList(va_list& tgt) : target(tgt), active(true) {}
string format_string(const char* fmt, Args const&... args) {
   return format_string_imp(fmt, unwrap(args)...);
 }
 
 error_code capture_errno() {
   _LIBCPP_ASSERT(errno, "Expected errno to be non-zero");
   return error_code(errno, generic_category());
 }
 
+#if defined(_LIBCPP_WIN32API)
+error_code make_windows_error(int err) {
+  return make_error_code(__win_err_to_errc(err));
+}
+#endif
+
 template <class T>
 T error_value();
 template <>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 void error_value<void>() {}
 template <>
 bool error_value<bool>() {
   return false;
 }
struct ErrorHandler {
   T report(const error_code& ec) const {
     if (ec_) {
       *ec_ = ec;
       return error_value<T>();
     }
     string what = string("in ") + func_name_;
     switch (bool(p1_) + bool(p2_)) {
     case 0:
       __throw_filesystem_error(what, ec);
     case 1:
       __throw_filesystem_error(what, *p1_, ec);
     case 2:
       __throw_filesystem_error(what, *p1_, *p2_, ec);
     }
     _LIBCPP_UNREACHABLE();
   }
 
   template <class... Args>
   T report(const error_code& ec, const char* msg, Args const&... args) const {
     if (ec_) {
       *ec_ = ec;
       return error_value<T>();
     }
     string what =

libcxx/src/filesystem/operations.cpp

string_view_t createView(PosPtr S, PosPtr E) noexcept {
   return {S, static_cast<size_t>(E - S) + 1};
 }
 
 } // namespace parser
 } // namespace
 
 //                       POSIX HELPERS
 
+#if defined(_LIBCPP_WIN32API)
+namespace detail {
+
+errc __win_err_to_errc(int err) {
+  constexpr struct {
+    DWORD win;
+    errc errc;
+  } win_error_mapping[] = {
+      {ERROR_ACCESS_DENIED, errc::permission_denied},
+      {ERROR_ALREADY_EXISTS, errc::file_exists},
+      {ERROR_BAD_NETPATH, errc::no_such_file_or_directory},
+      {ERROR_BAD_UNIT, errc::no_such_device},
+      {ERROR_BROKEN_PIPE, errc::broken_pipe},
+      {ERROR_BUFFER_OVERFLOW, errc::filename_too_long},
+      {ERROR_BUSY, errc::device_or_resource_busy},
+      {ERROR_BUSY_DRIVE, errc::device_or_resource_busy},
+      {ERROR_CANNOT_MAKE, errc::permission_denied},
+      {ERROR_CANTOPEN, errc::io_error},
+      {ERROR_CANTREAD, errc::io_error},
+      {ERROR_CANTWRITE, errc::io_error},
+      {ERROR_CURRENT_DIRECTORY, errc::permission_denied},
+      {ERROR_DEV_NOT_EXIST, errc::no_such_device},
+      {ERROR_DEVICE_IN_USE, errc::device_or_resource_busy},
+      {ERROR_DIR_NOT_EMPTY, errc::directory_not_empty},
+      {ERROR_DIRECTORY, errc::invalid_argument},
+      {ERROR_DISK_FULL, errc::no_space_on_device},
+      {ERROR_FILE_EXISTS, errc::file_exists},
+      {ERROR_FILE_NOT_FOUND, errc::no_such_file_or_directory},
+      {ERROR_HANDLE_DISK_FULL, errc::no_space_on_device},
+      {ERROR_INVALID_ACCESS, errc::permission_denied},
+      {ERROR_INVALID_DRIVE, errc::no_such_device},
+      {ERROR_INVALID_FUNCTION, errc::function_not_supported},
+      {ERROR_INVALID_HANDLE, errc::invalid_argument},
+      {ERROR_INVALID_NAME, errc::no_such_file_or_directory},
+      {ERROR_INVALID_PARAMETER, errc::invalid_argument},
+      {ERROR_LOCK_VIOLATION, errc::no_lock_available},
+      {ERROR_LOCKED, errc::no_lock_available},
+      {ERROR_NEGATIVE_SEEK, errc::invalid_argument},
+      {ERROR_NOACCESS, errc::permission_denied},
+      {ERROR_NOT_ENOUGH_MEMORY, errc::not_enough_memory},
+      {ERROR_NOT_READY, errc::resource_unavailable_try_again},
+      {ERROR_NOT_SAME_DEVICE, errc::cross_device_link},
+      {ERROR_NOT_SUPPORTED, errc::not_supported},
+      {ERROR_OPEN_FAILED, errc::io_error},
+      {ERROR_OPEN_FILES, errc::device_or_resource_busy},
+      {ERROR_OPERATION_ABORTED, errc::operation_canceled},
+      {ERROR_OUTOFMEMORY, errc::not_enough_memory},
+      {ERROR_PATH_NOT_FOUND, errc::no_such_file_or_directory},
+      {ERROR_READ_FAULT, errc::io_error},
+      {ERROR_REPARSE_TAG_INVALID, errc::invalid_argument},
+      {ERROR_RETRY, errc::resource_unavailable_try_again},
+      {ERROR_SEEK, errc::io_error},
+      {ERROR_SHARING_VIOLATION, errc::permission_denied},
+      {ERROR_TOO_MANY_OPEN_FILES, errc::too_many_files_open},
+      {ERROR_WRITE_FAULT, errc::io_error},
+      {ERROR_WRITE_PROTECT, errc::permission_denied},
+  };
+
+  for (const auto &pair : win_error_mapping)
+    if (pair.win == static_cast<DWORD>(err))
+      return pair.errc;
+  return errc::invalid_argument;
+}
+
+} // namespace detail
+#endif
+
 namespace detail {
 namespace {
 
 using value_type = path::value_type;
 using string_type = path::string_type;
 
 struct FileDescriptor {
   const path& name;