[libcxx-commits] [PATCH] D151493: [libcxx] Handle windows system error code mapping in std::error_code.

[James Y Knight via Phabricator via libcxx-commits]

jyknight added a comment.

In D151493#4373979 <https://reviews.llvm.org/D151493#4373979>, @mstorsjo wrote:

> 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 <https://github.com/llvm/llvm-project/blob/c5e6c886aabb36ab66b4ed835da243c2a3455ade/libcxx/src/filesystem/operations.cpp#L541>, 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 <https://github.com/llvm/llvm-project/blob/c5e6c886aabb36ab66b4ed835da243c2a3455ade/libcxx/include/__filesystem/directory_entry.h#L382> (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.



================
Comment at: libcxx/src/filesystem/operations.cpp:424
     if ((fd = detail::open(p->c_str(), args...)) == -1) {
-      ec = capture_errno();
+      ec = detail::get_last_error();
       return FileDescriptor{p};
----------------
mstorsjo wrote:
> 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.)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151493/new/

https://reviews.llvm.org/D151493