This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Handle windows system error code mapping in std::error_code.
Needs ReviewPublic

Authored by jyknight on May 25 2023, 1:46 PM.

Details

Reviewers
mstorsjo
Group Reviewers
Restricted Project
Summary

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 winerr.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.
  1. 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.

Diff Detail

Event Timeline

jyknight created this revision.May 25 2023, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 1:46 PM
jyknight requested review of this revision.May 25 2023, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 1:46 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Could you split the NFC changes into a separate patch?

libcxx/include/__filesystem/directory_entry.h
331–335

This should be equivalent, unless I'm missing something.

Looks mostly reasonable to me, but one rather significant issue about mapping between CRT and system error codes in the filesystem code.

One typo in the commit message, "winderr.h".

I don't quite follow all cases where errors can be compared directly and where they need to be canonicalized in the tests though.

libcxx/src/filesystem/operations.cpp
424

The detail::open() function uses _wopen(), which is a CRT function, which sets errno, not the windows error. (I guess it plausibly might leave the windows error code set too, but I guess we can't rely on it?)

So for all these cases where we switch from capture_errno() to get_last_error(), we'd need to make sure that we really do set the windows error code (or try to map from errno to windows error code).

(I guess the alternative, to keep track of which calls return in errno and which return in the windows error code and call the right function here, is unmaintainble.)

libcxx/src/system_error.cpp
31

Use lowercase windows.h for compatibility with mingw headers (which use lowercased header names consistently); while it is named Windows.h in the winsdk, the winsdk itself isn't self consistent and can't be used on a case sensitive filesystem without fixups in one form or another.

I don't quite follow all cases where errors can be compared directly and where they need to be canonicalized in the tests though.

The error_code comparison/canonicalization issue in the tests is isolated to directory_entry.

These tests are attempting to compare for equality the error_code you get from directory_entry.stat() to that you get from std::filesystem::stat(path), for a nonexistent path. However, directory_entry caches the absence of a file, and calls make_error_code(errc::no_such_file_or_directory);, instead of passing through the system's error code.

That is, in create_file_status, called from directory_entry::__do_refresh, it translates
m_ec == errc::no_such_file_or_directory || m_ec == errc::not_a_directory into file_type::not_found. Note that that comparison does the conversion from error_code to error_condition, which invokes system_category->generic_category conversion. Then, in __get_ft (and __get_sym_ft), it translates file_type::not_found into no_such_file_or_directory -- directly. So, we do lose which exact underlying error_code was originally reported.

I don't think this is likely to be a problem in practice, but it does break those test-cases.

libcxx/src/filesystem/operations.cpp
424

Ah, good catch, sorry I missed that! And, yeah, that is a significant problem.

One solution is to stop using the CRT functions at all in the filesystem impl layer for win32. That'll be a bit of work, but maybe it's not too bad. We'd abstract int fd into an internal file-handle type, (int on unix, HANDLE on win32) so we don't need open to return an int fd. Then, replace _wmkdir, _wopen, _close, _wchdir, and _wgetcwd. I think that's all. Seems doable -- and IMO, probably a good idea, anyways.

An alternative would be to return error_code from each of the detail::* functions. That way, it can be created from errno or GetLastError as required, right after the OS call in question.. The downside of that is that we'd need to add trivial wrapper functions on unix, instead of the current using ::open; etc.

jyknight added inline comments.Jun 7 2023, 2:06 PM
libcxx/src/filesystem/operations.cpp
424

Looking into this a bit more, we cannot actually migrate open/close away from fds. The only usage of open/close the std::filesystem impl is for copy_file, which (on windows) uses iostreams to actually do the copy -- which uses the [io]fstream::__open(int fd, ...) API. So, unless we _also_ migrated iostreams to using a HANDLE on windows instead of an fd, we still need to use fds (and thus libc's open) in the filesystem code.

I think the simplest fix is to go ahead and switch the other 3 calls over to native windows APIs, and simply remove open/close from the posix_compat.h layer (instead, just putting the #if inline in operations.cpp's FileDescriptor class -- the only user).

That way, everything in the the posix_compat abstraction layer is consistently returning errors via get_last_error.

jyknight updated this revision to Diff 531775.Jun 15 2023, 8:42 AM
jyknight marked 3 inline comments as done.

Rebase on top of new parent review, and address comments.

Could you split the NFC changes into a separate patch?

I haven't done this, because I'm not sure which changes you're referring to -- I don't think there's any separable NFC changes here.

Could you split the NFC changes into a separate patch?

I haven't done this, because I'm not sure which changes you're referring to -- I don't think there's any separable NFC changes here.

For example, moving __win_err_to_errc is NFC AFAICT.

For example, moving __win_err_to_errc is NFC AFAICT.

In the new code, __win_err_to_errc is a private implementation detail of std::system_error, living in an anonymous namespace inside system_error.cpp. In the old code, it's an extern function used by directory_iterator.cpp and operations.cpp within std::filesystem::detail. I _could_ kludge together a separate change which moves the source location of the code into system_error.cpp, while still having it be an extern symbol in the std::filesystem::detail namespace, but ISTM that's really stretching the bounds of a reasonable sort of NFC change to split off.

jyknight edited the summary of this revision. (Show Details)Jul 6 2023, 2:07 PM
mstorsjo accepted this revision.Jul 17 2023, 12:24 PM

Overall, this LGTM, but I'm not a libcxx approver.

libcxx/test/std/diagnostics/syserr/syserr.errcat/syserr.errcat.derived/message.pass.cpp
37

This changes what is tested for other platforms, previously this checked m1 != m3, now it checks m2 != m3. Although it is tested above that m1 == m2 though... (A comment about this change could be good, it's not immediately obvious to me what is fixed here.)