Fix tmp files being left on Windows builds.

[akhuang]

Summary

Clang writes object files by first writing to a .tmp file and then
renaming to the final .obj name. On Windows, if a compile is killed
partway through the .tmp files don't get deleted.

Currently it seems like RemoveFileOnSignal takes care of deleting the
tmp files on Linux, but on Windows we need to call
setDeleteDisposition on tmp files so that they are deleted when
closed.

This patch switches to using TempFile to create the .tmp files we write
when creating object files, since it uses setDeleteDisposition on Windows.
This change applies to both Linux and Windows for consistency.

[akhuang]

clang/lib/Frontend/CompilerInstance.cpp

882

I'm saving the windows HANDLE here instead of using fd because for some reason calling _get_osfhandle(fd) in UnmarkFileForDeletion fails.

It also doesn't work if I don't duplicate the handle, but I don't entirely understand why. (I think when we try to rename the file, it fails with file not found, so maybe it gets deleted?)

[aganea]

Do you think the existing crash tests can be modified to validate that .tmp files are deleted indeed?

clang/lib/Frontend/CompilerInstance.cpp

831

What do you think about having the comment inside the #idef _WIN32 ?

llvm/include/llvm/Support/FileSystem.h

994 ↗

(On Diff #346302)

It is unclear to me what is the right formatting for function names here? Is it what clang-tidy suggests or lowercase first, like the other functions?

[rnk]

For other reviewers, I know Duncan, @dexonsmith, has also spent some time working on the clang output file management APIs. And @jansvoboda11 as well, I think.

clang/lib/Frontend/CompilerInstance.cpp

720

This error is unchecked, so we don't know if the disposition set call succeeds.

831

Honestly, OF_Delete is documented as only working on Windows, so I think it would be fair to remove the ifdef, and keep this comment.

885

This API also returns an error code that must be checked.

[amccarth]

I'll have to dig into my archived email tomorrow.

I seem to recall some thrashing on this topic a few months ago. If I'm remembering correctly, setting the disposition to delete temporary files on Windows was causing problems with Rust builds because you can't always set the delete disposition (e.g., for a file on a network drive). I think it got pulled out, and then put back in in a limited way.

It certainly would be nice to clean up temporaries left because of a crash, but it's even better not to crash. :-) Lots of programs will leave partially written files if they die unexpectedly, and sometimes they can be useful when figuring out what happened. Part of me wonders whether we should reclaim orphaned temporaries on the next run instead. That might not be hard if we had a predictable location and naming scheme.

Another crazy idea would be to have the driver sit idly while the compiler and linker do their thing, and then clean up behind them if necessary.

Tests, however, always need to be cleaned up one way or another.

I'll take a closer look in the morning.

[aganea]

In D102736#2767516, @amccarth wrote:

I seem to recall some thrashing on this topic a few months ago. If I'm remembering correctly, setting the disposition to delete temporary files on Windows was causing problems with Rust builds because you can't always set the delete disposition (e.g., for a file on a network drive). I think it got pulled out, and then put back in in a limited way.

It landed in rG64ab2b6825c5aeae6e4afa7ef0829b89a6828102 - it fixed the initial Rust issue. I can add a Windows 7 bot if you think it's worth it. But I'm happy to occasionally maintain the Win7 port for as long as it's needed.

[amccarth]

At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done.

(I find it weird that duplicating the handle seems necessary.)

At a high level, it seems a shame that llvm::support::fs doesn't have create-temporary-file and keep-temporary-file operations to hide all this detail from the frontend.

clang/lib/Frontend/CompilerInstance.cpp

721

IIUC, OF.Handle is the duplicate handle, and we're done with it at this point. It should be closed, before doing things like trying to rename/move the file.

[amccarth]

Sorry, forgot to Request Changes.

[akhuang]

In D102736#2769358, @amccarth wrote:

At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done.

(I find it weird that duplicating the handle seems necessary.)

At a high level, it seems a shame that llvm::support::fs doesn't have create-temporary-file and keep-temporary-file operations to hide all this detail from the frontend.

So, there is a TempFile class in llvm::support::fs with create temp file and keep temp file operations, and it would be nice to use that. The issue I was running into is that it uses a file descriptor (int fd) to keep track of the file, but for some reason, the function to convert the fd to a handle (_get_osfhandle) was not working (exits the program when called). I haven't yet figured out why this is happening

[akhuang]

In D102736#2769747, @akhuang wrote:

In D102736#2769358, @amccarth wrote:

At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done.

(I find it weird that duplicating the handle seems necessary.)

At a high level, it seems a shame that llvm::support::fs doesn't have create-temporary-file and keep-temporary-file operations to hide all this detail from the frontend.

So, there is a TempFile class in llvm::support::fs with create temp file and keep temp file operations, and it would be nice to use that. The issue I was running into is that it uses a file descriptor (int fd) to keep track of the file, but for some reason, the function to convert the fd to a handle (_get_osfhandle) was not working (exits the program when called). I haven't yet figured out why this is happening

Ok, apparently the whole issue with it not being able to open the handle is just because we were passing shouldClose=true to the raw_fd_ostream, which makes it close the file when done writing.
Since that's figured out, I think using TempFile makes the most sense

[akhuang]

Change to using TempFiles

[aganea]

clang/include/clang/Frontend/CompilerInstance.h

170

Can we always use TempFile? And remove TempFilename in that case?

clang/lib/Frontend/CompilerInstance.cpp

729

I think the idea overall in LLVM is to defer the error rendering as late as possible. You could probably only keep an Error variable here, and render it below in the diagnostic with toString(E).

734

Here you can probably use ECError or errorCodeToError() with what is said slightly above.

829–830

Usually it is better to avoid platform-specific logic. Could we use TempFile all the time on all platforms?

[amccarth]

Cool! This is getting much simpler. My remaining comments are mostly musings and you can tell me to buzz off.

But I'd like to see @aganea's questions addressed. It does seem redundant to have to keep the file name when we already have an object that represents the file.

llvm/lib/Support/Path.cpp

1237

I'm curious if this path has ever been exercised.

I see that rename_handle is implemented with MoveFileExW on Windows. MoveFileExW docs say it will fail if you're trying to move a _directory_ to another device, but that it can do copy+delete to move a _file_ to another device. (That might require adding MOVEFILE_COPY_ALLOWED to the flags passed in the MoveFileExW in lib\Support\Windows\Path.inc near line 485.)

Anyway, it just seems like we're re-implementing functionality the OS calls can already do for us.

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw

1243

If I understand correctly, we're using the delete disposition flag to get the OS to clean up the file in case we crash. But does that prevent us from just deleting it outright once we know it's safe to do so?

In other words if we could just delete the file here, then the divergence between Win32 and others could possibly be reduced.

1253

Possibly stupid idea: What if RemoveFileOnSignal and DontRemoveFileOnSignal did the Win32-specific delete disposition flag twiddling? With that encapsulated, and assuming we can still explicitly delete a Windows file that's already marked for deletion, it seems like all of the special handling here could go away.

Then again, the ...OnSignal things are in sys rather than sys::fs, so maybe it's not such a good idea.

I'm not trying to make more work for Amy. The immediate goal probably isn't well served by this level of overthinking. But it would be nice to see all this get simpler in the long term.

[akhuang]

In D102736#2767432, @aganea wrote:

Do you think the existing crash tests can be modified to validate that .tmp files are deleted indeed?

Looks like the existing crash tests use #pragma clang __debug crash but in the case where clang crashes it's actually fine. I guess the only cases where this patch actually applies is when the program is killed -- don't know if there's a good way to do that in the test suite?

clang/lib/Frontend/CompilerInstance.cpp

734

I think this path goes away if we always use TempFile

829–830

Hm, probably, that would make things simpler.

llvm/lib/Support/Path.cpp

1253

@rnk suggested that too - I think it would definitely make things nicer, but yeah, the fact that it's in sys and not sys::fs is inconvenient.

[akhuang]

Use TempFile on both linux and windows

[akhuang]

llvm/lib/Support/Path.cpp

1237

according to https://reviews.llvm.org/D50048 we used to pass MOVEFILE_COPY_ALLOWED but then removed it because it calls rename_internal which fails.

[rnk]

Nice, this seems to be working out.

clang/include/clang/Frontend/CompilerInstance.h

170

Having a field and type both named TempFile is setting off my portability alarms. I think some old versions of GCC don't like this. Maybe the field can be called Temporary? TemporaryFile? I dono.

clang/lib/Frontend/CompilerInstance.cpp

709–710

Can this be consumeError(OF.TempFile->discard())?

I find consumeError hard to read, I wish it were ignoreErrors or something like that. Short of that, please add a comment at the top of this function that we ignore any errors about failing to remove files.

720–721

Ditto re consumeError simplification

729

ErrorMsg seems unused now

734

Can you skip the toString and just do << std::move(E)? I see an operator<< overload for it:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Diagnostic.h#L1518

llvm/lib/Support/Path.cpp

1264

Please revert unrelated whitespace changes

[akhuang]

address comments

[rnk]

I think this look good. Adrian, are your concerns addressed?

[amccarth]

LGTM.

Yeah, this is good. My remaining comments are all speculations about how to improve this further in the future, but they aren't directly applicable to the goal here.

[akhuang]

Committed in 9d070b2f4889887f9ce497592ef01df7b9601a1c

[dyung]

Hi Amy, after your change, the compile time for one of our internal test files went from about 1 second to 30 seconds. I have put a repro and the details in PR50628, can you please take a look?

[akhuang]

yep, I'll look into it, probably sometime tomorrow--

[rnk]

clang/lib/Frontend/CompilerInstance.cpp

857–858

I think the bug is here: the third parameter is bool unbuffered, not file flags, so we are opening the file for unbuffered writing, and that is really slow.

[amccarth]

Nice catch Reid.

clang/lib/Frontend/CompilerInstance.cpp

857–858

Yowza! In addition to a fix, we need some memes.

• Why do we even have that lever?
• This constructor has too many overloads. Please remove three.
• [Facepalm] bool parameters. [Double facepalm] Negative bool parameter names that default to false.

[• Quuxplusone]

clang/lib/Frontend/CompilerInstance.cpp

857–858

In addition to a fix, we need some memes.

https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/ ;)
I count 8 different signatures for raw_fd_ostream's constructor, 3 of which come from the offending default-argument'ed overload. And none of them are explicit, which means e.g. {42, false} is a valid argument for a function taking const raw_fd_ostream&.

Unrelated to argument lists, but I would add "using raw new instead of std::make_unique" to this line's lengthy list of sins.

[akhuang]

clang/lib/Frontend/CompilerInstance.cpp

857–858

oh whoops, thanks for the catch - for now I guess I'll just remove the extra parameter

clang/include/clang/Frontend/CompilerInstance.h

 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/BuryPointer.h"
+#include "llvm/Support/FileSystem.h"
 #include <cassert>
 #include <list>
 #include <memory>
 #include <string>
 #include <utility>
 
 namespace llvm {
 class raw_fd_ostream;
class CompilerInstance : public ModuleLoader {
 
   /// Holds information about the output file.
   ///
   /// If TempFilename is not empty we must rename it to Filename at the end.
   /// TempFilename may be empty and Filename non-empty if creating the temporary
   /// failed.
   struct OutputFile {
     std::string Filename;
-    std::string TempFilename;
+    Optional<llvm::sys::fs::TempFile> File;
 
-    OutputFile(std::string filename, std::string tempFilename)
-        : Filename(std::move(filename)), TempFilename(std::move(tempFilename)) {
-    }
+    OutputFile(std::string filename, Optional<llvm::sys::fs::TempFile> file)
+        : Filename(std::move(filename)), File(std::move(file)) {}
   };
 
   /// The list of active output files.
   std::list<OutputFile> OutputFiles;
 
   /// Force an output buffer.
   std::unique_ptr<llvm::raw_pwrite_stream> OutputStream;
 

clang/lib/Frontend/CompilerInstance.cpp

if (ExternalSemaSrc) {
     TheSema->addExternalSource(ExternalSemaSrc.get());
     ExternalSemaSrc->InitializeSema(*TheSema);
   }
 }
 
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
     if (EraseFiles) {
-      if (!OF.TempFilename.empty()) {
-        llvm::sys::fs::remove(OF.TempFilename);
-        continue;
-      }
+      if (OF.File)
+        consumeError(OF.File->discard());
       if (!OF.Filename.empty())
         llvm::sys::fs::remove(OF.Filename);
       continue;
     }
 
-    if (OF.TempFilename.empty())
+    if (!OF.File)
+      continue;
+
+    if (OF.File->TmpName.empty()) {
+      consumeError(OF.File->discard());
       continue;
+    }
 
     // If '-working-directory' was passed, the output filename should be
     // relative to that.
     SmallString<128> NewOutFile(OF.Filename);
     FileMgr->FixupRelativePath(NewOutFile);
-    std::error_code EC = llvm::sys::fs::rename(OF.TempFilename, NewOutFile);
-    if (!EC)
+
+    llvm::Error E = OF.File->keep(NewOutFile);
+    if (!E)
       continue;
+
     getDiagnostics().Report(diag::err_unable_to_rename_temp)
-        << OF.TempFilename << OF.Filename << EC.message();
+        << OF.File->TmpName << OF.Filename << std::move(E);
 
-    llvm::sys::fs::remove(OF.TempFilename);
+    llvm::sys::fs::remove(OF.File->TmpName);
   }
   OutputFiles.clear();
   if (DeleteBuiltModules) {
     for (auto &Module : BuiltModules)
       llvm::sys::fs::remove(Module.second);
     BuiltModules.clear();
   }
 }
else {
         // Don't use a temporary if the output is a special file. This handles
         // things like '-o /dev/null'
         if (!llvm::sys::fs::is_regular_file(Status))
           UseTemporary = false;
       }
     }
   }
 
-  std::string TempFile;
+  Optional<llvm::sys::fs::TempFile> Temp;
   if (UseTemporary) {
     // Create a temporary file.
     // Insert -%%%%%%%% before the extension (if any), and because some tools
     // (noticeable, clang's own GlobalModuleIndex.cpp) glob for build
     // artifacts, also append .tmp.
     StringRef OutputExtension = llvm::sys::path::extension(OutputPath);
     SmallString<128> TempPath =
         StringRef(OutputPath).drop_back(OutputExtension.size());
     TempPath += "-%%%%%%%%";
     TempPath += OutputExtension;
     TempPath += ".tmp";
-    int fd;
-    std::error_code EC = llvm::sys::fs::createUniqueFile(
-        TempPath, fd, TempPath,
-        Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text);
+    Expected<llvm::sys::fs::TempFile> ExpectedFile =
+        llvm::sys::fs::TempFile::create(TempPath);
 
+    llvm::Error E = handleErrors(
+        ExpectedFile.takeError(), [&](const llvm::ECError &E) -> llvm::Error {
+          std::error_code EC = E.convertToErrorCode();
           if (CreateMissingDirectories &&
               EC == llvm::errc::no_such_file_or_directory) {
             StringRef Parent = llvm::sys::path::parent_path(OutputPath);
             EC = llvm::sys::fs::create_directories(Parent);
             if (!EC) {
-        EC = llvm::sys::fs::createUniqueFile(TempPath, fd, TempPath,
-                                             Binary ? llvm::sys::fs::OF_None
-                                                    : llvm::sys::fs::OF_Text);
+              ExpectedFile = llvm::sys::fs::TempFile::create(TempPath);
+              if (!ExpectedFile)
+                return llvm::errorCodeToError(
+                    llvm::errc::no_such_file_or_directory);
             }
           }
+          return llvm::errorCodeToError(EC);
+        });
 
-    if (!EC) {
-      OS.reset(new llvm::raw_fd_ostream(fd, /*shouldClose=*/true));
-      OSFile = TempFile = std::string(TempPath.str());
+    if (E) {
+      consumeError(std::move(E));
+    } else {
+      Temp = std::move(ExpectedFile.get());
+      TempPath = Temp->TmpName;
+
+      OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false,
+                                        Binary ? llvm::sys::fs::OF_None
+                                               : llvm::sys::fs::OF_Text));
+      OSFile = std::string(TempPath.str());
     }
     // If we failed to create the temporary, fallback to writing to the file
     // directly. This handles the corner case where we cannot write to the
     // directory, but can write to the file.
   }
 
   if (!OS) {
     OSFile = OutputPath;
     std::error_code EC;
     OS.reset(new llvm::raw_fd_ostream(
         *OSFile, EC,
         (Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_TextWithCRLF)));
     if (EC)
       return llvm::errorCodeToError(EC);
   }
 
   // Make sure the out stream file gets removed if we crash.
   if (RemoveFileOnSignal)
     llvm::sys::RemoveFileOnSignal(*OSFile);
 
   // Add the output file -- but don't try to remove "-", since this means we are
   // using stdin.
   OutputFiles.emplace_back(((OutputPath != "-") ? OutputPath : "").str(),
-                           std::move(TempFile));
+                           std::move(Temp));
 
   if (!Binary || OS->supportsSeeking())
     return std::move(OS);
 
   return std::make_unique<llvm::buffer_unique_ostream>(std::move(OS));
 }
 
 // Initialization Utilities
 
 bool CompilerInstance::InitializeSourceManager(const FrontendInputFile &Input){

llvm/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) {
-    RenameEC = rename_fd(FD, Name);
+    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);
     }
   }
 
   // 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);
     // If we can't rename or copy, discard the temporary file.
     if (RenameEC)
       remove(TmpName);
   }
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
 
   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

llvm/lib/Support/Windows/Path.inc

for (unsigned Retry = 0; Retry != 200; ++Retry) {
     // process might have raced with us to create and open the destination
     // file, so we need to keep doing this until we succeed.
   }
 
   // The most likely root cause.
   return errc::permission_denied;
 }
 
-static std::error_code rename_fd(int FromFD, const Twine &To) {
-  HANDLE FromHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FromFD));
-  return rename_handle(FromHandle, To);
-}
-
 std::error_code rename(const Twine &From, const Twine &To) {
   // Convert to utf-16.
   SmallVector<wchar_t, 128> WideFrom;
   if (std::error_code EC = widenPath(From, WideFrom))
     return EC;
 
   ScopedFileHandle FromHandle;
   // Retry this a few times to defeat badly behaved file system scanners.