[Support] [Windows] Use the regular RemoveFileOnSignal() mechanism on Windows too

[mstorsjo]

Summary

This effectively reverts 3ecd20430cace7eb9e78ef0a9ff9cc95a377feb9
(together with some further fixes from
881ba104656c40098d4bc90c52613c08136f0fe1).

The original change away from RemoveFileOnSignal() didn't say why
what was gained, other than "We won't see the temp file no more.".

All use of FILE_FLAG_DELETE_ON_CLOSE or FILE_DISPOSITION_INFO
with DeleteFile=true, and the DELETE access flag, cause various
issues. The original FILE_FLAG_DELETE_ON_CLOSE (in
3ecd20430cace7eb9e78ef0a9ff9cc95a377feb9) had to be worked around
when deciding to keep the file (to cancel the effect of the flag).
This was reworked in 881ba104656c40098d4bc90c52613c08136f0fe1 / D48051
as that approach broke some cases when memory mapping and closing
files (as used in LTO).

This further broke cases when writing temp files on network shares,
worked around in 79657e2339b58bc01fe1b85a448bb073d57d90bb / D81803,
which then broke some cases on Windows 7, worked around in
64ab2b6825c5aeae6e4afa7ef0829b89a6828102. After that, the temp
files were left behind on network shares, even after calling
TempFile::discard(). Fixes for this were attempted in D111875,
but would end up needing to set the DELETE access flag when opening
files for writing in general (or further splitting up/changing the
OF_Delete flag), which breaks when opening a file multiple times
(see discussion in D111875).

Thus simply backtrack and abandon the whole use of delete-on-close
flags on Windows, and use the same RemoveFileOnSignal() mechanism
as on Unix. (The RemoveFileOnSignal() mechanism was still used for
other temp files, not created by TempFile::create.)

After this change, one could maybe also reinstante
51c63bb7efff5f90cd3894dc2b6f73a614dbfb96 (which was reverted as
part of 881ba104656c40098d4bc90c52613c08136f0fe1 / D48051), to
use TempFile in the implementation of LockFileManager.

[mstorsjo]

Fixed lld/test/ELF/link-open-file.test (renaming over a currently open file) by keeping more of the Windows specific renaming logic (using rename_handle instead of plain rename).

[akhuang]

I think on Windows the RemoveFileOnSignal path doesn't always work when the process is killed, which causes temp files to be left sometimes (see https://reviews.llvm.org/D102736)

[mstorsjo]

In D112376#3088384, @akhuang wrote:

I think on Windows the RemoveFileOnSignal path doesn't always work when the process is killed, which causes temp files to be left sometimes (see https://reviews.llvm.org/D102736)

Oh, thanks. That's indeed a good reason for keeping the existing approach, although it seems to become a giant pile of hacks/workarounds... I'll see what I can do on the other patch again, then.

[rnk]

In D112376#3088384, @akhuang wrote:

I think on Windows the RemoveFileOnSignal path doesn't always work when the process is killed, which causes temp files to be left sometimes (see https://reviews.llvm.org/D102736)

Yeah, at least in our usage, we found RemoveFileOnSignal was unreliable. The fancy delete disposition feature seemed like exactly what we wanted, it's just a shame that it only seems to work on local NTFS volumes.

I was going to try to come up with one last alternative, something like splitting OF_Delete into OF_DeleteOnClose and OF_Delete. One gives you the ability to delete the file, and the other is the old delete-on-close semantic. Does that work? Sorry for the delay, apologies if I missed something while skimming...

[mstorsjo]

In D112376#3088498, @rnk wrote:

I was going to try to come up with one last alternative, something like splitting OF_Delete into OF_DeleteOnClose and OF_Delete. One gives you the ability to delete the file, and the other is the old delete-on-close semantic. Does that work? Sorry for the delay, apologies if I missed something while skimming...

On second thought, we don't even need to split it - we could just change the OF_Delete flag into "just set the delete access mode, nothing else" and call setDeleteDisposition directly from TempFile::create. Then we could either keep the approach of checking is_local for deciding whether we should call setDeleteDisposition, or go back to making setDeleteDisposition update a reference parameter about whether it did something or not. (But those bool reference returns are a bit ugly, so unless there's some unforseen issue with upfront checking with is_local, maybe that's cleaner.)

[andrewevstyukhin]

Hi,
in https://reviews.llvm.org/D81803 I showed that is_local_internal had no chances to execute because

if (std::error_code EC = realPathFromHandle(Handle, FinalPath))

failed with error 40 (ERROR_INVALID_FUNCTION, function_not_supported). This happens at RAM-disk https://sourceforge.net/projects/imdisk-toolkit/.
Currently emscripten versions 2.0.9+ couldn't pass tests at RAM-disk and versions 2.0.19+ couldn't even link anything when temporaries are at RAM-disk.

[mstorsjo]

In D112376#3089544, @andrewevstyukhin wrote:

Hi,
in https://reviews.llvm.org/D81803 I showed that is_local_internal had no chances to execute because

if (std::error_code EC = realPathFromHandle(Handle, FinalPath))

failed with error 40 (ERROR_INVALID_FUNCTION, function_not_supported). This happens at RAM-disk https://sourceforge.net/projects/imdisk-toolkit/.
Currently emscripten versions 2.0.9+ couldn't pass tests at RAM-disk and versions 2.0.19+ couldn't even link anything when temporaries are at RAM-disk.

Hmm, I think this would work with D111875 then, if I’d update it to remove the check for the specific error code on the return from setDeleteDisposition?

[mstorsjo]

Abandoning this in favour of D111875.

[jhenderson]

Thanks everyone for looking at this (see also the very recent https://xkcd.com/2531/).

llvm/include/llvm/Support/FileSystem.h

enum OpenFlags : unsigned {
   /// The file should be opened in text mode and use a carriage linefeed '\r\n'.
   /// This flag has the same functionality as OF_Text on z/OS but adds a
   /// carriage linefeed on Windows.
   OF_TextWithCRLF = OF_Text | OF_CRLF,
 
   /// The file should be opened in append mode.
   OF_Append = 4,
 
-  /// Delete the file on close. Only makes a difference on windows.
+  /// The returned handle can be used for deleting the file. Only makes a
+  /// difference on windows.
   OF_Delete = 8,
 
   /// When a child process is launched, this file should remain open in the
   /// child process.
   OF_ChildInherit = 16,
 
   /// Force files Atime to be updated on access. Only makes a difference on
   /// Windows.

llvm/lib/Support/Path.cpp

 Error TempFile::discard() {
   Done = true;
   if (FD != -1 && close(FD) == -1) {
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
-#ifdef _WIN32
-  // On windows closing will remove the file.
-  TmpName = "";
-  return Error::success();
-#else
   // Always try to close and remove.
   std::error_code RemoveEC;
   if (!TmpName.empty()) {
     RemoveEC = fs::remove(TmpName);
     sys::DontRemoveFileOnSignal(TmpName);
     if (!RemoveEC)
       TmpName = "";
   }
   return errorCodeToError(RemoveEC);
-#endif
 }
 
 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) {
-    RenameEC = rename_handle(H, 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);
-    }
-  }
-
+  std::error_code RenameEC = rename_handle(H, Name);
+    // Regardless of whether the copy succeeded or not, we remove the original.
+    remove(TmpName);
+  } else if (RenameEC)
     // If we can't rename, discard the temporary file.
-  if (RenameEC)
-    setDeleteDisposition(H, true);
+    remove(TmpName);
 #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);
     // If we can't rename or copy, discard the temporary file.
     if (RenameEC)
       remove(TmpName);
   }
-  sys::DontRemoveFileOnSignal(TmpName);
 #endif
+  sys::DontRemoveFileOnSignal(TmpName);
 
   if (!RenameEC)
     TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return errorCodeToError(RenameEC);
 }
 
 Error TempFile::keep() {
   assert(!Done);
   Done = true;
 
-#ifdef _WIN32
-  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-  if (std::error_code EC = setDeleteDisposition(H, false))
-    return errorCodeToError(EC);
-#else
   sys::DontRemoveFileOnSignal(TmpName);
-#endif
 
   TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return Error::success();
 }
 
 Expected<TempFile> TempFile::create(const Twine &Model, unsigned Mode,
                                     OpenFlags ExtraFlags) {
   int FD;
   SmallString<128> ResultPath;
   if (std::error_code EC =
           createUniqueFile(Model, FD, ResultPath, OF_Delete | ExtraFlags, Mode))
     return errorCodeToError(EC);
 
   TempFile Ret(ResultPath, FD);
-#ifndef _WIN32
   if (sys::RemoveFileOnSignal(ResultPath)) {
     // Make sure we delete the file when RemoveFileOnSignal fails.
     consumeError(Ret.discard());
     std::error_code EC(errc::operation_not_permitted);
     return errorCodeToError(EC);
   }
-#endif
   return std::move(Ret);
 }
 } // namespace fs
 
 } // namespace sys
 } // namespace llvm

llvm/lib/Support/Windows/Path.inc

std::error_code is_local(int FD, bool &Result) {
   HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
 
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   return is_local_internal(FinalPath, Result);
 }
 
-static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
-  // Clear the FILE_DISPOSITION_INFO flag first, before checking if it's a
-  // network file. On Windows 7 the function realPathFromHandle() below fails
-  // if the FILE_DISPOSITION_INFO flag was already set to 'DeleteFile = true' by
-  // a prior call.
-  FILE_DISPOSITION_INFO Disposition;
-  Disposition.DeleteFile = false;
-  if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
-                                  sizeof(Disposition)))
-    return mapWindowsError(::GetLastError());
-  if (!Delete)
-    return std::error_code();
-
-  // Check if the file is on a network (non-local) drive. If so, don't
-  // continue when DeleteFile is true, since it prevents opening the file for
-  // writes. Note -- this will leak temporary files on disk, but only when the
-  // target file is on a network drive.
-  SmallVector<wchar_t, 128> FinalPath;
-  if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
-    return EC;
-
-  bool IsLocal;
-  if (std::error_code EC = is_local_internal(FinalPath, IsLocal))
-    return EC;
-
-  if (!IsLocal)
-    return std::error_code();
-
-  // The file is on a local drive, we can safely set FILE_DISPOSITION_INFO's
-  // flag.
-  Disposition.DeleteFile = true;
-  if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
-                                  sizeof(Disposition)))
-    return mapWindowsError(::GetLastError());
-  return std::error_code();
-}
-
 static std::error_code rename_internal(HANDLE FromHandle, const Twine &To,
                                        bool ReplaceIfExists) {
   SmallVector<wchar_t, 0> ToWide;
   if (auto EC = widenPath(To, ToWide))
     return EC;
 
   std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) - sizeof(wchar_t) +
                                   (ToWide.size() * sizeof(wchar_t)));
if (Flags & OF_UpdateAtime) {
     if (SystemTimeToFileTime(&SystemTime, &FileTime) == 0 ||
         SetFileTime(Result, NULL, &FileTime, NULL) == 0) {
       DWORD LastError = ::GetLastError();
       ::CloseHandle(Result);
       return errorCodeToError(mapWindowsError(LastError));
     }
   }
 
-  if (Flags & OF_Delete) {
-    if ((EC = setDeleteDisposition(Result, true))) {
-      ::CloseHandle(Result);
-      return errorCodeToError(EC);
-    }
-  }
   return Result;
 }
 
 std::error_code openFile(const Twine &Name, int &ResultFD,
                          CreationDisposition Disp, FileAccess Access,
                          OpenFlags Flags, unsigned int Mode) {
   Expected<file_t> Result = openNativeFile(Name, Disp, Access, Flags);
   if (!Result)