[Windows FS] Allow moving files in TempFile::keep

[jmorse]

Summary

In r338216 and https://reviews.llvm.org/D49860 we permitted MoveFileEx to copy between volumes -- but before it can, the more posix-y rename_internal fails. Allow rename_handle to fall back on MoveFileEx for this specific error.

[JDevlieghere]

LGTM, Thanks for fixing this!

[zturner]

I don't have a strong opinion, but I wonder if it would be better to map ERROR_NOT_SAME_DEVICE to errc::cross_device_link and to map ERROR_CALL_NOT_IMPLEMENTED to errc::operation_not_supported. Then you could just say if (EC == errc::cross_device_link || EC == errc::operation_not_supported)

[stella.stamenova]

Tests are all good. Thanks!

[pcc]

Can't we change dsymutil to create its temporary file in the same directory as the destination?

[zturner]

In D50048#1183101, @pcc wrote:

Can't we change dsymutil to create its temporary file in the same directory as the destination?

I'm not sure that's the best solution. Even if we fix it for dsymutil, this seems to be a more general problem of LLVM not supporting cross-device renames. We can't control where a user's temp dir will be or what drive they're running LLVM from. So I think it's important to handle this case.

[pcc]

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

[jmorse]

In D50048#1183136, @pcc wrote:

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

My understanding of the TempFile API is that it might not know whether a file is kept or the destination it might be kept to, at the time of creation.

IMHO, this comes down the support code not currently providing a "move", hence this patch. However with this patch unix-rename and windows-rename might behave differently.

The more technically consistent solution could be to mirror Jonas' cross-device copy in https://reviews.llvm.org/D49860 TempFile::keep for Windows. The problem is then just code duplication (and a lack of "move"), rather than functions behaving differently.

I'll upload a patch that does that in a moment to demonstrate.

[jmorse]

This revision works just as well as the previous, but keeps "rename" behaving the same. Some future refactor could introduce a "move" function, this is the minimum change for now.

lib/Support/Path.cpp

 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
 #ifdef _WIN32
   // If we can't cancel the delete don't rename.
   auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   std::error_code RenameEC = setDeleteDisposition(H, false);
-  if (!RenameEC)
+  if (!RenameEC) {
     RenameEC = rename_fd(FD, Name);
+    // If rename failed because it's cross-device, copy instead
+    if (RenameEC ==
+      std::error_code(ERROR_NOT_SAME_DEVICE, std::system_category())) {
+      RenameEC = copy_file(TmpName, Name);
+      setDeleteDisposition(H, true);
+    }
+  }
+
   // If we can't rename, discard the temporary file.
   if (RenameEC)
     setDeleteDisposition(H, true);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   if (RenameEC) {
     // If we can't rename, try to copy to work around cross-device link issues.
     RenameEC = sys::fs::copy_file(TmpName, Name);

lib/Support/Windows/Path.inc

for (unsigned Retry = 0; Retry != 200; ++Retry) {
     if (EC ==
         std::error_code(ERROR_CALL_NOT_IMPLEMENTED, std::system_category())) {
       // Wine doesn't support SetFileInformationByHandle in rename_internal.
       // Fall back to MoveFileEx.
       SmallVector<wchar_t, MAX_PATH> WideFrom;
       if (std::error_code EC2 = realPathFromHandle(FromHandle, WideFrom))
         return EC2;
       if (::MoveFileExW(WideFrom.begin(), WideTo.begin(),
-                        MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED))
+                        MOVEFILE_REPLACE_EXISTING))
         return std::error_code();
       return mapWindowsError(GetLastError());
     }
 
     if (!EC || EC != errc::permission_denied)
       return EC;
 
     // The destination file probably exists and is currently open in another