[libcxx] Handle windows system error code mapping in std::error_code. #93101
Conversation
The std::error_code/std::error_category functionality is designed to support multiple error domains. On Unix, both system calls and libc functions return the same error codes, and thus, libc++ today treats generic_category() and system_category() as being equivalent. However, on Windows, libc functions return `errno.h` error codes in the `errno` global, but system calls return the very different `winerror.h` error codes via `GetLastError()`. As such, there is a need to map the winerror.h error codes into generic errno codes. In libc++, however, the system_error facility does not currently implement this mapping, and instead the mapping is hidden inside libc++, used directly by the std::filesystem implementation. That has a few problems: 1. For std::filesystem APIs, the concrete windows error number is lost, before users can see it. The intent of the distinction between std::error_code and std::error_condition is that the error_code return has the original (potentially more detailed) error code. 2. User-written code which calls Windows system APIs requires this same mapping, so it also can also return error_code objects that other (cross-platform) code can understand. After this commit, an `error_code` with `generic_category()` is used to report an error from `errno`, and, on Windows only, an `error_code` with `system_category()` is used to report an error from `GetLastError()`. On Unix, system_category remains identity-mapped to generic_category, but is never used by libc++ itself. The windows error code mapping is moved into system_error, so that conversion of an `error_code` to `error_condition` correctly translates the `system_category()` code into a `generic_category()` code, when appropriate. This allows code like: `error_code(GetLastError(), system_category()) == errc::invalid_argument` to work as expected -- as it does with MSVC STL. Differential Revision: https://reviews.llvm.org/D151493
…or-codes-3 Resolved conflicts: libcxx/src/system_error.cpp libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp
Resolved conflicts: libcxx/src/filesystem/operations.cpp libcxx/test/std/diagnostics/syserr/syserr.compare/eq_error_code_error_code.pass.cpp libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.derived/message.pass.cpp libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.objects/system_category.pass.cpp
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: James Y Knight (jyknight) ChangesThe std::error_code/std::error_category functionality is designed to support multiple error domains. On Unix, both system calls and libc functions return the same error codes, and thus, libc++ today treats generic_category() and system_category() as being equivalent. However, on Windows, libc functions return As such, there is a need to map the winerror.h error codes into generic errno codes. In libc++, however, the system_error facility does not currently implement this mapping, and instead the mapping is hidden inside libc++, used directly by the std::filesystem implementation. That has a few problems:
After this commit, an The windows error code mapping is moved into system_error, so that conversion of an This allows code like: (Continued from old phabricator review D151493) Patch is 39.58 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93101.diff 15 Files Affected:
diff --git a/libcxx/include/__filesystem/directory_entry.h b/libcxx/include/__filesystem/directory_entry.h
index 016ad94a853dc..ef10acfa2dbab 100644
--- a/libcxx/include/__filesystem/directory_entry.h
+++ b/libcxx/include/__filesystem/directory_entry.h
@@ -22,6 +22,7 @@
#include <__filesystem/path.h>
#include <__filesystem/perms.h>
#include <__system_error/errc.h>
+#include <__system_error/error_category.h>
#include <__system_error/error_code.h>
#include <__utility/move.h>
#include <__utility/unreachable.h>
@@ -249,15 +250,7 @@ class directory_entry {
_LIBCPP_EXPORTED_FROM_ABI error_code __do_refresh() noexcept;
_LIBCPP_HIDE_FROM_ABI static bool __is_dne_error(error_code const& __ec) {
- if (!__ec)
- return true;
- switch (static_cast<errc>(__ec.value())) {
- case errc::no_such_file_or_directory:
- case errc::not_a_directory:
- return true;
- default:
- return false;
- }
+ return !__ec || __ec == errc::no_such_file_or_directory || __ec == errc::not_a_directory;
}
_LIBCPP_HIDE_FROM_ABI void
diff --git a/libcxx/include/__system_error/system_error.h b/libcxx/include/__system_error/system_error.h
index 362e67505658c..ab0718860a86e 100644
--- a/libcxx/include/__system_error/system_error.h
+++ b/libcxx/include/__system_error/system_error.h
@@ -39,6 +39,8 @@ class _LIBCPP_EXPORTED_FROM_ABI system_error : public runtime_error {
_LIBCPP_HIDE_FROM_ABI const error_code& code() const _NOEXCEPT { return __ec_; }
};
+// __ev is expected to be an error in the generic_category domain (e.g. from
+// errno, or std::errc::*), not system_category (e.g. from windows syscalls).
_LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_system_error(int __ev, const char* __what_arg);
_LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI inline void __throw_system_error(error_code __ec, const char* __what_arg) {
#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
diff --git a/libcxx/src/filesystem/directory_iterator.cpp b/libcxx/src/filesystem/directory_iterator.cpp
index dceb3486279f8..402de65bbf2f3 100644
--- a/libcxx/src/filesystem/directory_iterator.cpp
+++ b/libcxx/src/filesystem/directory_iterator.cpp
@@ -47,9 +47,9 @@ class __dir_stream {
}
__stream_ = ::FindFirstFileW((root / "*").c_str(), &__data_);
if (__stream_ == INVALID_HANDLE_VALUE) {
- ec = detail::make_windows_error(GetLastError());
+ ec = detail::get_last_error();
const bool ignore_permission_denied = bool(opts & directory_options::skip_permission_denied);
- if (ignore_permission_denied && ec.value() == static_cast<int>(errc::permission_denied))
+ if (ignore_permission_denied && ec == errc::permission_denied)
ec.clear();
return;
}
@@ -91,7 +91,7 @@ class __dir_stream {
error_code close() noexcept {
error_code ec;
if (!::FindClose(__stream_))
- ec = detail::make_windows_error(GetLastError());
+ ec = detail::get_last_error();
__stream_ = INVALID_HANDLE_VALUE;
return ec;
}
@@ -118,7 +118,7 @@ class __dir_stream {
if ((__stream_ = ::opendir(root.c_str())) == nullptr) {
ec = detail::capture_errno();
const bool allow_eacces = bool(opts & directory_options::skip_permission_denied);
- if (allow_eacces && ec.value() == EACCES)
+ if (allow_eacces && ec == errc::permission_denied)
ec.clear();
return;
}
@@ -307,7 +307,7 @@ bool recursive_directory_iterator::__try_recursion(error_code* ec) {
}
if (m_ec) {
const bool allow_eacess = bool(__imp_->__options_ & directory_options::skip_permission_denied);
- if (m_ec.value() == EACCES && allow_eacess) {
+ if (m_ec == errc::permission_denied && allow_eacess) {
if (ec)
ec->clear();
} else {
diff --git a/libcxx/src/filesystem/error.h b/libcxx/src/filesystem/error.h
index 572cc73292a19..9429bc1493aca 100644
--- a/libcxx/src/filesystem/error.h
+++ b/libcxx/src/filesystem/error.h
@@ -32,80 +32,21 @@ _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
namespace detail {
-#if defined(_LIBCPP_WIN32API)
-
-inline 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_PATHNAME, 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;
-}
-
-#endif // _LIBCPP_WIN32API
+// On windows, libc functions use errno, but system functions use GetLastError.
+// So, callers need to be careful which of these next functions they call!
inline error_code capture_errno() {
_LIBCPP_ASSERT_INTERNAL(errno != 0, "Expected errno to be non-zero");
return error_code(errno, generic_category());
}
+inline error_code get_last_error() {
#if defined(_LIBCPP_WIN32API)
-inline error_code make_windows_error(int err) { return make_error_code(__win_err_to_errc(err)); }
+ return std::error_code(GetLastError(), std::system_category());
+#else
+ return capture_errno();
#endif
+}
template <class T>
T error_value();
diff --git a/libcxx/src/filesystem/file_descriptor.h b/libcxx/src/filesystem/file_descriptor.h
index 50178ff84e03f..9fce904028979 100644
--- a/libcxx/src/filesystem/file_descriptor.h
+++ b/libcxx/src/filesystem/file_descriptor.h
@@ -194,7 +194,7 @@ inline perms posix_get_perms(const StatT& st) noexcept { return static_cast<perm
inline file_status create_file_status(error_code& m_ec, path const& p, const StatT& path_stat, error_code* ec) {
if (ec)
*ec = m_ec;
- if (m_ec && (m_ec.value() == ENOENT || m_ec.value() == ENOTDIR)) {
+ if (m_ec && (m_ec == errc::no_such_file_or_directory || m_ec == errc::not_a_directory)) {
return file_status(file_type::not_found);
} else if (m_ec) {
ErrorHandler<void> err("posix_stat", ec, &p);
@@ -229,7 +229,7 @@ inline file_status create_file_status(error_code& m_ec, path const& p, const Sta
inline file_status posix_stat(path const& p, StatT& path_stat, error_code* ec) {
error_code m_ec;
if (detail::stat(p.c_str(), &path_stat) == -1)
- m_ec = detail::capture_errno();
+ m_ec = detail::get_last_error();
return create_file_status(m_ec, p, path_stat, ec);
}
@@ -241,7 +241,7 @@ inline file_status posix_stat(path const& p, error_code* ec) {
inline file_status posix_lstat(path const& p, StatT& path_stat, error_code* ec) {
error_code m_ec;
if (detail::lstat(p.c_str(), &path_stat) == -1)
- m_ec = detail::capture_errno();
+ m_ec = detail::get_last_error();
return create_file_status(m_ec, p, path_stat, ec);
}
@@ -253,7 +253,7 @@ inline file_status posix_lstat(path const& p, error_code* ec) {
// http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html
inline bool posix_ftruncate(const FileDescriptor& fd, off_t to_size, error_code& ec) {
if (detail::ftruncate(fd.fd, to_size) == -1) {
- ec = capture_errno();
+ ec = get_last_error();
return true;
}
ec.clear();
@@ -262,7 +262,7 @@ inline bool posix_ftruncate(const FileDescriptor& fd, off_t to_size, error_code&
inline bool posix_fchmod(const FileDescriptor& fd, const StatT& st, error_code& ec) {
if (detail::fchmod(fd.fd, st.st_mode) == -1) {
- ec = capture_errno();
+ ec = get_last_error();
return true;
}
ec.clear();
@@ -279,7 +279,7 @@ inline file_status FileDescriptor::refresh_status(error_code& ec) {
m_stat = {};
error_code m_ec;
if (detail::fstat(fd, &m_stat) == -1)
- m_ec = capture_errno();
+ m_ec = get_last_error();
m_status = create_file_status(m_ec, name, m_stat, &ec);
return m_status;
}
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 62bb248d5eca9..f2928ab661457 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -86,7 +86,7 @@ path __canonical(path const& orig_p, error_code* ec) {
#if (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112) || defined(_LIBCPP_WIN32API)
std::unique_ptr<path::value_type, decltype(&::free)> hold(detail::realpath(p.c_str(), nullptr), &::free);
if (hold.get() == nullptr)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
return {hold.get()};
#else
# if defined(__MVS__) && !defined(PATH_MAX)
@@ -96,7 +96,7 @@ path __canonical(path const& orig_p, error_code* ec) {
# endif
path::value_type* ret;
if ((ret = detail::realpath(p.c_str(), buff)) == nullptr)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
return {ret};
#endif
}
@@ -396,9 +396,9 @@ bool __create_directory(const path& p, error_code* ec) {
if (detail::mkdir(p.c_str(), static_cast<int>(perms::all)) == 0)
return true;
- if (errno != EEXIST)
- return err.report(capture_errno());
- error_code mec = capture_errno();
+ error_code mec = detail::get_last_error();
+ if (mec != errc::file_exists)
+ return err.report(mec);
error_code ignored_ec;
const file_status st = status(p, ignored_ec);
if (!is_directory(st))
@@ -420,10 +420,10 @@ bool __create_directory(path const& p, path const& attributes, error_code* ec) {
if (detail::mkdir(p.c_str(), attr_stat.st_mode) == 0)
return true;
- if (errno != EEXIST)
- return err.report(capture_errno());
+ mec = detail::get_last_error();
+ if (mec != errc::file_exists)
+ return err.report(mec);
- mec = capture_errno();
error_code ignored_ec;
st = status(p, ignored_ec);
if (!is_directory(st))
@@ -434,19 +434,19 @@ bool __create_directory(path const& p, path const& attributes, error_code* ec) {
void __create_directory_symlink(path const& from, path const& to, error_code* ec) {
ErrorHandler<void> err("create_directory_symlink", ec, &from, &to);
if (detail::symlink_dir(from.c_str(), to.c_str()) == -1)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
void __create_hard_link(const path& from, const path& to, error_code* ec) {
ErrorHandler<void> err("create_hard_link", ec, &from, &to);
if (detail::link(from.c_str(), to.c_str()) == -1)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
void __create_symlink(path const& from, path const& to, error_code* ec) {
ErrorHandler<void> err("create_symlink", ec, &from, &to);
if (detail::symlink_file(from.c_str(), to.c_str()) == -1)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
path __current_path(error_code* ec) {
@@ -489,7 +489,7 @@ path __current_path(error_code* ec) {
unique_ptr<path::value_type, Deleter> hold(detail::getcwd(ptr, size), deleter);
if (hold.get() == nullptr)
- return err.report(capture_errno(), "call to getcwd failed");
+ return err.report(detail::get_last_error(), "call to getcwd failed");
return {hold.get()};
}
@@ -497,7 +497,7 @@ path __current_path(error_code* ec) {
void __current_path(const path& p, error_code* ec) {
ErrorHandler<void> err("current_path", ec, &p);
if (detail::chdir(p.c_str()) == -1)
- err.report(capture_errno());
+ err.report(detail::get_last_error());
}
bool __equivalent(const path& p1, const path& p2, error_code* ec) {
@@ -585,10 +585,10 @@ void __last_write_time(const path& p, file_time_type new_time, error_code* ec) {
return err.report(errc::value_too_large);
detail::WinHandle h(p.c_str(), FILE_WRITE_ATTRIBUTES, 0);
if (!h)
- return err.report(detail::make_windows_error(GetLastError()));
+ return err.report(detail::get_last_error());
FILETIME last_write = timespec_to_filetime(ts);
if (!SetFileTime(h, nullptr, nullptr, &last_write))
- return err.report(detail::make_windows_error(GetLastError()));
+ return err.report(detail::get_last_error());
#else
error_code m_ec;
array<TimeSpec, 2> tbuf;
@@ -646,7 +646,7 @@ void __permissions(const path& p, perms prms, perm_options opts, error_code* ec)
#if defined(AT_SYMLINK_NOFOLLOW) && defined(AT_FDCWD)
const int flags = set_sym_perms ? AT_SYMLINK_NOFOLLOW : 0;
if (detail::fchmodat(AT_FDCWD, p.c_str(), real_perms, flags) == -1) {
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
#else
if (set_sym_perms)
@@ -674,14 +674,14 @@ path __read_symlink(const path& p, error_code* ec) {
#else
StatT sb;
if (detail::lstat(p.c_str(), &sb) == -1) {
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
const size_t size = sb.st_size + 1;
auto buff = unique_ptr<path::value_type[]>(new path::value_type[size]);
#endif
detail::SSizeT ret;
if ((ret = detail::readlink(p.c_str(), buff.get(), size)) == -1)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
// Note that `ret` returning `0` would work, resulting in a valid empty string being returned.
if (static_cast<size_t>(ret) >= size)
return err.report(errc::value_too_large);
@@ -692,8 +692,9 @@ path __read_symlink(const path& p, error_code* ec) {
bool __remove(const path& p, error_code* ec) {
ErrorHandler<bool> err("remove", ec, &p);
if (detail::remove(p.c_str()) == -1) {
- if (errno != ENOENT)
- err.report(capture_errno());
+ error_code mec = detail::get_last_error();
+ if (mec != errc::no_such_file_or_directory)
+ err.report(mec);
return false;
}
return true;
@@ -846,13 +847,13 @@ uintmax_t __remove_all(const path& p, error_code* ec) {
void __rename(const path& from, const path& to, error_code* ec) {
ErrorHandler<void> err("rename", ec, &from, &to);
if (detail::rename(from.c_str(), to.c_str()) == -1)
- err.report(capture_errno());
+ err.report(detail::get_last_error());
}
void __resize_file(const path& p, uintmax_t size, error_code* ec) {
ErrorHandler<void> err("resize_file", ec, &p);
if (detail::truncate(p.c_str(), static_cast< ::off_t>(size)) == -1)
- return err.report(capture_errno());
+ return err.report(detail::get_last_error());
}
space_info __space(const path& p, error_code* ec) {
@@ -860,7 +861,7 @@ space_info __space(const path& p, error_code* ec) {
space_info si;
detail::StatVFS m_svfs = {};
if (detail::statvfs(p.c_str(), &m_svfs) == -1) {
- err.report(capture_errno());
+ err.report(detail::get_last_error());
si.capacity = si.free = si.available = static_cast<uintmax_t>(-1);
return si;
}
@@ -887,7 +888,7 @@ path __temp_directory_path(error_code* ec) {
wchar_t buf[MAX_PATH];
DWORD retval = GetTempPathW(MAX_PATH, buf);
if (!retval)
- return err.report(detail::make_windows_error(GetLastError()));
+ return err.report(detail::get_last_error());
if (retval > MAX_PATH)
return err.report(errc::filename_too_long);
// GetTempPathW returns a path with a trailing slash, which we
diff --git a/libcxx/src/filesystem/posix_compat.h b/libcxx/src/filesystem/posix_compat.h
index 760cdb65dae1d..dba65e2a7691b 100644
--- a/libcxx/src/filesystem/posix_compat.h
+++ b/libcxx/src/filesystem/posix_compat.h
@@ -11,9 +11,10 @@
//
// 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().
+// - 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().
+// - Errors should be retrieved with get_last_error, not errno.
//
// These are provided within an anonymous namespace within the detail
// namespace - callers need to include this header and call them as
@@ -122,11 +123,6 @@ namespace detail {
# define O_NONBLOCK 0
-inline int set_errno(int e = GetLastError()) {
- errno = static_cast<int>(__win_err_to_errc(e));
- return -1;
-}
-
class WinHandle {
public:
WinHandle(const wchar_t* p, DWORD access, DWORD flags) {
@@ -153,7 +149,7 @@ class WinHandle {
inline int stat_handle(HANDLE h, StatT* buf) {
FILE_BASIC_INFO basic;
if (!GetFileInformationByHandleEx(h, FileBasicInfo, &basic, sizeof(basic)))
- return set_errno();
+ return -1;
memset(buf, 0, sizeof(*buf));
buf->st_mtim = filetime_to_timespec(basic.LastWriteTime);
buf->st_atim = filetime_to_timespec(basic.LastAccessTime);
@@ -168,18 +164,18 @@ inline int stat_handle(HANDLE h, StatT* buf) {
if (basic.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
FILE_ATTRIBUTE_TAG_INFO tag;
if (!GetFileInformationByHandleEx(h, FileAttributeTagInfo, &tag, sizeof(tag)))
- return set_errno();
+ return -1;
if (tag.ReparseTag == IO_REPARSE_TAG_SYMLINK)
buf->st_mode = (buf->st_mode & ~_S_IFMT) | _S_IFLNK;
}
FILE_STANDARD_INFO standard;
if (!GetFileInformationByHandleEx(h, FileStandardInfo, &standard, sizeof(standard)))
- return set_errno();
+ return -1;
buf->st_nlink = standard.NumberOfLinks;
buf->st_size = standard.EndOfFile.QuadPart;
BY_HANDLE_FILE_INFORMATION info;
if (!GetFileInformationByHandle(h, &info))
- return set_errno();
+ return -1;
buf->st_dev = info.dwVolumeSerialNumber;
memcpy(&buf->st_ino.id[0], &info.nFileIndexHigh, 4);
memcpy(&buf->st_ino.id[4], &info.nFileIndexLow, 4);
@@ -189,7 +185,7 @@ inline int stat_handle(HANDLE h, StatT* buf) {
inline int stat_file(const wchar_t* path, StatT* buf, DWORD flags) {
WinHandle h(path, FILE_READ_ATTRIBUTES, flags);
if (!h)
- return set_errno();
+ return -1;
int ret = stat_handle(h, buf);
return ret;
}
@@ -206,7 +202,7 @@ inline int fstat(int fd, StatT* buf) {
inline int mkdir(const wchar_t* path, int permissions) {
(void)permissions;
if (!Creat...
[truncated]
|
|
Ping! |
There was a problem hiding this comment.
This generally looks sensible to me, but I have some questions and a few comments. I would also like to confirm that this is an ABI break because we'll return different error codes from the dylib before/after this patch, right? But that's only on Windows where we're not ABI stable anyways, so that shouldn't be an issue.
Is there a possibility that this would break user code that is relying on the previous mapping, or does that previous mapping never escape out of libc++ implementation details?
| @@ -300,15 +296,15 @@ inline int statvfs(const wchar_t* p, StatVFS* buf) { | |||
| break; | |||
| path parent = dir.parent_path(); | |||
| if (parent == dir) { | |||
| errno = ENOENT; | |||
| SetLastError(ERROR_PATH_NOT_FOUND); | |||
There was a problem hiding this comment.
Why do you SetLastError here but everywhere else you only return -1? Same question a few times below.
There was a problem hiding this comment.
This function is now expected to report its error to the caller via get_last_error(). Formerly, it was expected to report its error to the caller via errno.
In most cases, the error is coming from the Windows API itself (e.g. the cases above), so it's already in the right place to be reported by get_last_error. Where it previously called set_errno, to convert from windows error code to errno, now doesn't need to do anything.
But, on this line, and a few places below, there is no underlying OS API call that is setting the error code. These cases formerly manually set errno, and now equivalently need to set the windows error code (via SetLastError).
libcxx/src/system_error.cpp
Outdated
| #ifdef _LIBCPP_WIN32API | ||
| std::string result; | ||
| char* str = nullptr; | ||
| unsigned long num_chars = FormatMessageA( |
There was a problem hiding this comment.
| unsigned long num_chars = FormatMessageA( | |
| unsigned long num_chars = ::FormatMessageA( |
That makes it clearer that we're calling a Windows system function.
libcxx/src/system_error.cpp
Outdated
| if (ev == 0) | ||
| return error_condition(0, generic_category()); | ||
| if (auto maybe_errc = __win_err_to_errc(ev)) | ||
| return error_condition(static_cast<int>(maybe_errc.value()), generic_category()); |
There was a problem hiding this comment.
| return error_condition(static_cast<int>(maybe_errc.value()), generic_category()); | |
| return error_condition(static_cast<int>(*maybe_errc), generic_category()); |
You just checked that the optional was engaged, so we can use * here.
| #ifdef _WIN32 | ||
| // Windows' system error 5 is ERROR_ACCESS_DENIED, which maps to generic code permission_denied. | ||
| LIBCPP_ASSERT(e_cond.value() == static_cast<int>(std::errc::permission_denied)); | ||
| #else |
There was a problem hiding this comment.
So IIUC this test was always making an incorrect assumption, it just happened to work because our mapping between the system and generic categories was the identity function. Right?
There was a problem hiding this comment.
Yep. system_category::default_error_condition is supposed to create an error_candition in generic_category (if possible), using the system_category value specified. There's no guarantee that system_category value 5 maps to generic_category value 5, but it did before.
| // We compare the canonicalized error_conditions for this case, because | ||
| // directory_entry may construct its own generic_category error when a | ||
| // file doesn't exist, instead of directly returning a system_category | ||
| // error. |
There was a problem hiding this comment.
| // We compare the canonicalized error_conditions for this case, because | |
| // directory_entry may construct its own generic_category error when a | |
| // file doesn't exist, instead of directly returning a system_category | |
| // error. | |
| // We compare the canonicalized error_conditions because | |
| // directory_entry may construct its own generic_category error when a | |
| // file doesn't exist, instead of directly returning a system_category | |
| // error. |
What does for this case refer to? The way this comment was worded is a bit fishy -- it feels a bit as-if the comment was interesting to reviewers of this patch but not necessarily to someone reading this code a year down the line.
There was a problem hiding this comment.
The point is that, in other tests we do test exact error_code equality, but here we don't. I've tried to reword it to make that clearer.
| @@ -582,7 +582,11 @@ struct ExceptionChecker { | |||
| assert(ErrorIsImp(Err.code(), {expected_err})); | |||
There was a problem hiding this comment.
Not attached to this file:
In the PR description, you mention:
This allows code like:
error_code(GetLastError(), system_category()) == errc::invalid_argumentto work as expected -- as it does with MSVC STL.
I don't think I saw any test added to validate that this worked as intended. Did I miss that or should we add a test?
There was a problem hiding this comment.
Good point -- the behavior is being tested implicitly, but not explicitly. Added a new explicit test in libcxx/test/libcxx/diagnostics/system_error_win_codes.pass.cpp.
I'd like to clarify this statement - as there are two separate ABIs on Windows. In the MSVC/clang-cl setup, we're not ABI stable, we have multiple breaks for various changes, and the users of this setup have agreed that it's not an issue for them. For the MinGW/Itanium cases, we do have users that generally appreciate ABI stability if possible (e.g. there's the whole msys2 package manager that have thousands of binary packages built with libc++). Still it's not a strict case where all potential aspects and corner cases of ABIs must be kept at all costs, but it's not a case where we should feel free to break the ABI willy-nilly whenever we feel like it. As for this particular case - returning different error codes than we used to before, I don't think that's an ABI aspect that really should be an issue. Packages built with older libc++ should still run but just get different error codes now, right? |
|
Gentle ping @jyknight, I can carve out some time to get this reviewed again if you update it. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Yes, there is: user could could be expecting that |
|
Hm. There's a new failure now...it looks like in the "generic-modules" builder, only, clang fails to emit the function definition for |
There was a problem hiding this comment.
This LGTM with my few comments resolved and a release note.
About the CI issue:
Hm. There's a new failure now...it looks like in the "generic-modules" builder, only, clang fails to emit the function definition for template <class _Ep, __enable_if_t<is_error_condition_enum<_Ep>::value, int> = 0> _LIBCPP_HIDE_FROM_ABI error_condition(_Ep __e) _NOEXCEPT, even though it's required by __is_dne_error (or, maybe emits it under a different mangling?). Not sure why this would happen -- anyone else know what might cause this error?
I would suggest rebasing this after #107638 lands, it cleans up modules and includes a whole lot. I suspect the issue would be easier to understand after that.
| @@ -201,7 +201,7 @@ inline perms posix_get_perms(const StatT& st) noexcept { return static_cast<perm | |||
| inline file_status create_file_status(error_code& m_ec, path const& p, const StatT& path_stat, error_code* ec) { | |||
There was a problem hiding this comment.
Not attached to this line: Can you add a release note in libcxx/docs/ReleaseNotes/20.rst?
|
|
||
| int main(int, char**) { | ||
| LIBCPP_ASSERT(std::error_code(ERROR_ACCESS_DENIED, std::system_category()) == std::errc::permission_denied); | ||
| LIBCPP_ASSERT(std::error_code(ERROR_PATH_NOT_FOUND, std::system_category()) == std::errc::no_such_file_or_directory); |
Merging that seems to have fixed the mysterious error, yay! |
|
Ping! |
The std::error_code/std::error_category functionality is designed to support multiple error domains. On Unix, both system calls and libc functions return the same error codes, and thus, libc++ today treats generic_category() and system_category() as being equivalent.
However, on Windows, libc functions return
errno.herror codes in theerrnoglobal, but system calls return the very differentwinerror.herror codes viaGetLastError().As such, there is a need to map the winerror.h error codes into generic errno codes. In libc++, however, the system_error facility does not currently implement this mapping, and instead the mapping is hidden inside libc++, used directly by the std::filesystem implementation.
That has a few problems:
For std::filesystem APIs, the concrete windows error number is lost, before users can see it. The intent of the distinction between std::error_code and std::error_condition is that the error_code return has the original (potentially more detailed) error code.
User-written code which calls Windows system APIs requires this same mapping, so it also can also return error_code objects that other (cross-platform) code can understand.
After this commit, an
error_codewithgeneric_category()is used to report an error fromerrno, and, on Windows only, anerror_codewithsystem_category()is used to report an error fromGetLastError(). On Unix, system_category remains identity-mapped to generic_category, but is never used by libc++ itself.The windows error code mapping is moved into system_error, so that conversion of an
error_codetoerror_conditioncorrectly translates thesystem_category()code into ageneric_category()code, when appropriate.This allows code like:
error_code(GetLastError(), system_category()) == errc::invalid_argumentto work as expected -- as it does with MSVC STL.
(Continued from old phabricator review D151493)