[Support] Fix TempFile::discard to not leave behind temporary files

[andrewng]

Summary

Moved the remove of the temporary file to after the close to avoid
remove failures caused by ETXTBSY errors.

This issue was seen when FileOutputBuffer falls back to an in memory
buffer due to the inability to mmap the on disk file. This occurred when
running LLD on an Ubuntu VM in VirtualBox on a Windows host attempting
to write the output to a VirtualBox shared folder.

[ruiu]

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

[andrewng]

In D57960#1390773, @ruiu wrote:

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

Actually, that was my first approach to fix this issue; to move the remove after the close. However, I then recalled that doing the file remove (unlink) before the close is actually the "normal" approach for POSIX (perhaps because it's more "atomic" in behaviour), so I changed to this implementation instead. But if moving the remove after the close is acceptable, I'm happy for that approach.

[ruiu]

I think that unlinking a file while opening a file is a common way to create an unnamed temporary file. By doing that, you don't have to worry about leaving temporary files behind. However, if your temporary file can be permanent by renaming it to a permanent filename, that method is irrelevant because you can't unlink a file (if you do, your file become unnamed, and there's no POSIX-compliant way to revive unnamed file as a named one AFAIK.)

What we are trying to do here is the latter, so I think the order of removing a file and closing a file descriptor doesn't matter.

[andrewng]

As suggested, I have switched back to what was my original change to fix this issue.

[ruiu]

lib/Support/Path.cpp

1131

Now the rest of the code is virtuall a no-op on Windows, so you can wrap it entirely with an ifdef like this, which should improve readability.

#ifdef _WIN32
  // On windows closing will remove the file.
  TmpName = ""
  return Error::success();
#else
  std::error_code RemoveEC;
    // Always try to close and remove.
  if (!TmpName.empty()) {
    RemoveEC = fs::remove(TmpName);
    sys::DontRemoveFileOnSignal(TmpName);
  }
  if (!RemoveEC)
    TmpName = "";
  return errorCodeToError(RemoveEC);
#endif

[andrewng]

Updated the patch based on review suggestion.

[ruiu]

LGTM

lib/Support/Path.cpp

TempFile &TempFile::operator=(TempFile &&Other) {
   Other.Done = true;
   return *this;
 }
 
 TempFile::~TempFile() { assert(Done); }
 
 Error TempFile::discard() {
   Done = true;
   std::error_code RemoveEC;
 // On windows closing will remove the file.
 #ifndef _WIN32
   // Always try to close and remove.
   if (!TmpName.empty()) {
     RemoveEC = fs::remove(TmpName);
     sys::DontRemoveFileOnSignal(TmpName);
   }
 #endif
 
   if (!RemoveEC)
     TmpName = "";
 
   if (FD != -1 && close(FD) == -1) {
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
+#ifndef _WIN32
+  if (RemoveEC.value() == ETXTBSY) {
+    RemoveEC = fs::remove(TmpName);
+    if (!RemoveEC)
+      TmpName = "";
+  }
+#endif
+
   return errorCodeToError(RemoveEC);
 }
 
 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
 #ifdef _WIN32