[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 inline comments.


================
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};
----------------
jyknight wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151493