This is an archive of the discontinued LLVM Phabricator instance.

[11/N] [libcxx] Hook up a number of operation functions to their windows counterparts
ClosedPublic

Authored by mstorsjo on Nov 10 2020, 1:46 AM.

Details

Summary

Use the corresponding wchar functions, named "_wfunc" instead of "func", where feasible, or reimplement functions with native windows APIs.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 1:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 1:46 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk resigned from this revision.Nov 10 2020, 12:26 PM

I have a couple questions to consider inline. If those have already been thought through, then LTGM.

libcxx/src/filesystem/operations.cpp
520

Technically, there's nothing wrong here. It just _feels_ a little sketchy to call GetLastError() twice in a row.

541

Why not implement this with ::DeleteFile()? Does it have to do with waiting until the last handle is closed?

563

I'm mildly concerned about the way files are sometimes manipulated by HANDLE and sometimes by file descriptor.

If fd represents a file in text mode, then the caller needs to be very careful to understand that length must be the _byte_ offset, not the _char_ offset into the file.

575

Consider also MOVEFILE_WRITE_THROUGH for the case that it is indeed copied and then (probably) deleted.

mstorsjo added inline comments.Nov 12 2020, 2:20 PM
libcxx/src/filesystem/operations.cpp
520

Yeah, it's not ideal, but then again, making a separate version of the set_errno() function that takes an error code as input, just for this case, also feels a bit superfluous (as the first call to GetLastError() shouldn't clobber the first one). Or would a default parameter, like set_errno(int err = GetLastError()) work?

541

Haven't considered/tested that, I can try that and run it over the testcases to see if it works.

The reason I ended up with this, was that I first just called _wremove(), followed by a _wrmdir() if the first failed and the path is a directory - as that was the most obvious path forward when porting over from the posix remove() function. That worked mostly, but failed in some symlink cases, and then I peeked at how the MS STL (which has the same license as libcxx, fwiw) implemented it, and I did the same.

563

File descriptors here are only an internal detail, fwiw, they're only used within the __copy_file function. But good catch about text vs binary mode; I guess the calls to FileDescriptor::create_with_status need to be amended with O_BINARY.

575

Sounds like a good thing to have, I can add that. These flags were based on what the MS STL use, and I didn't see that flag used there though.

mstorsjo marked 3 inline comments as done.Nov 13 2020, 3:29 AM
mstorsjo added inline comments.
libcxx/src/filesystem/operations.cpp
520

Changed set_errno() to take the error code as a parameter, with the parameter set by default to GetLastError().

541

I tried with this implementation, but this still fails on removing symlinks to directories:

int remove(const wchar_t *path) {
  if (DeleteFileW(path))
    return 0;
  int e = GetLastError();
  if (e == ERROR_ACCESS_DENIED) {
    if (is_directory(path)) {
      if (!RemoveDirectoryW(path)) 
        return set_errno();
      return 0;
    }
  }
  return set_errno(e);
}
563

Adding O_BINARY at the callsites that open file descriptors.

575

Adding that flag as it sounds beneficial, despite MS STL not using it.

mstorsjo updated this revision to Diff 305080.Nov 13 2020, 3:43 AM

Made symlink_file_dir pass the inspected error code to set_errno(), passing O_BINARY to the open() function, passing the MOVEFILE_WRITE_THROUGH flag to MoveFileExW.

amccarth accepted this revision.Dec 2 2020, 3:15 PM

LGTM, now.

mstorsjo updated this revision to Diff 312722.Dec 18 2020, 1:39 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, already formally approved.

mstorsjo updated this revision to Diff 317574.Jan 19 2021, 8:19 AM

Rebased, moved helpers to posix_compat.h.

mstorsjo updated this revision to Diff 317576.Jan 19 2021, 8:20 AM

Retrigger CI.

ldionne accepted this revision.Jan 28 2021, 3:17 PM
ldionne added a subscriber: ldionne.

The integration between libc++ and the Windows shim looks great. I didn't look at the actual implementations using Windows APIs (I don't know those APIs and I trust @amccarth with that). LGTM once CI passes (can you please rebase to trigger, I'll add the LLVM monorepo to the diff now).

This revision is now accepted and ready to land.Jan 28 2021, 3:17 PM
ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 3:17 PM
mstorsjo updated this revision to Diff 320066.Jan 29 2021, 12:38 AM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, rerunning CI before pushing

This revision was landed with ongoing or failed builds.Jan 29 2021, 3:40 AM
This revision was automatically updated to reflect the committed changes.