<div dir="ltr">I agree, having a nameless FD would be the most elegant way to solve this problem on Unix, but when I investigated it, I came to the same conclusion that you did: it can't be done portably. Nameless files are just too weird, so I think we're stuck with RemoveFileOnSignal. =/<div><br></div><div>For Windows, I was able to use SetFileInformationByHandle to get the OS to clean up files for us on exit. I attached my proof-of-concept program that does this.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 9, 2017 at 1:13 PM, Rafael Avila de Espindola via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Currently a power failure or other hard crash can cause lld leave a temporary<br>
file around. The same is true for other llvm tools.<br>
<br>
As an example, put a breakpoint in Writer.cpp:236 ("writeBuildId()") and<br>
restart the run a few times. You will get<br>
<br>
t.tmp43a735a  t.tmp4deeabb  t.tmp9bacdd3  t.tmpe4115c4  t.tmpeb01fff<br>
<br>
The same would happen if there was a fatal error between the<br>
FileOutputBuffer creation and commit. I don't think that is a code path<br>
where that is possible right now, but it would be an easy thing to miss<br>
in a code review.<br>
<br>
I was hopping the OS could help us manage the temporary file so that<br>
there was no way to accidentally leave it behind.<br>
<br>
On linux there is O_TMPFILE, which allows us to create a file with no<br>
name in the file system. A name can be given with linkat. Unfortunately<br>
we can't use<br>
<br>
linkat(fd, "", AT_FDCWD, "destination", AT_EMPTY_PATH)<br>
<br>
Without special permissions and have instead to depend on proc:<br>
<br>
linkat(AT_FDCWD, "/proc/self/fd/<num>", AT_FDCWD, "destination",<br>
       AT_SYMLINK_FOLLOW)<br>
<br>
Another annoyance is that linkat will not replace the destination and<br>
renameat2 doesn't support AT_EMPTY_PATH. The result is that we have to<br>
use unlink+linkat and loop.<br>
<br>
On windows there is FILE_FLAG_DELETE_ON_CLOSE, but there seems to be no<br>
way to cancel it. If a file is created with it I can rename it, but it<br>
is still deleted in the end.<br>
<br>
I couldn't find any support for this on FreeBSD.<br>
<br>
This suggest that we cannot just have a createUniqueEntity that returs<br>
just an FD. We will need a class that stores a temporary name too for<br>
the systems where we cannot give a filename to a FD.<br>
<br>
I have attached the patch I got so far, but given the existing<br>
restrictions I would say it is not worth it.<br>
<br>
It will try to just make it more obvious that lld's FileOutputBuffer is<br>
deleted on all code paths.<br>
<br>
<br>diff --git a/include/llvm/Support/<wbr>FileSystem.h b/include/llvm/Support/<wbr>FileSystem.h<br>
index 03015a0ca3b..5d80db1d5a2 100644<br>
--- a/include/llvm/Support/<wbr>FileSystem.h<br>
+++ b/include/llvm/Support/<wbr>FileSystem.h<br>
@@ -31,6 +31,7 @@<br>
 #include "llvm/ADT/StringRef.h"<br>
 #include "llvm/ADT/Twine.h"<br>
 #include "llvm/Support/Chrono.h"<br>
+#include "llvm/Support/Error.h"<br>
 #include "llvm/Support/ErrorHandling.h"<br>
 #include "llvm/Support/ErrorOr.h"<br>
 #include "llvm/Support/MD5.h"<br>
@@ -694,6 +695,26 @@ std::error_code createUniqueFile(const Twine &Model, int &ResultFD,<br>
 std::error_code createUniqueFile(const Twine &Model,<br>
                                  SmallVectorImpl<char> &ResultPath);<br>
<br>
+// Represents a temporary file and hides the name. This allows not even having<br>
+// no name when O_TMPFILE is supported.<br>
+class TempFile {<br>
+  // Temporary name used when O_TMPFILE is not avilable.<br>
+  std::string TmpName;<br>
+<br>
+public:<br>
+  TempFile(StringRef Name, int FD);<br>
+  TempFile(TempFile &&Other);<br>
+<br>
+  // The open file descriptor.<br>
+  int FD;<br>
+<br>
+  Error keep(const Twine &Name);<br>
+  ~TempFile();<br>
+};<br>
+<br>
+Expected<TempFile> createUniqueFile(const Twine &Directory,<br>
+                                    unsigned Mode = all_read | all_write);<br>
+<br>
 /// @brief Create a file in the system temporary directory.<br>
 ///<br>
 /// The filename is of the form prefix-random_chars.suffix. Since the directory<br>
diff --git a/lib/Support/<wbr>FileOutputBuffer.cpp b/lib/Support/<wbr>FileOutputBuffer.cpp<br>
index db8ff38e5b5..af61af3c32e 100644<br>
--- a/lib/Support/<wbr>FileOutputBuffer.cpp<br>
+++ b/lib/Support/<wbr>FileOutputBuffer.cpp<br>
@@ -34,9 +34,9 @@ using namespace llvm::sys;<br>
 // with the temporary file on commit().<br>
 class OnDiskBuffer : public FileOutputBuffer {<br>
 public:<br>
-  OnDiskBuffer(StringRef Path, StringRef TempPath,<br>
+  OnDiskBuffer(StringRef Path, fs::TempFile Temp,<br>
                std::unique_ptr<fs::mapped_<wbr>file_region> Buf)<br>
-      : FileOutputBuffer(Path), Buffer(std::move(Buf)), TempPath(TempPath) {}<br>
+      : FileOutputBuffer(Path), Buffer(std::move(Buf)), Temp(std::move(Temp)) {}<br>
<br>
   uint8_t *getBufferStart() const override { return (uint8_t *)Buffer->data(); }<br>
<br>
@@ -51,21 +51,18 @@ public:<br>
     Buffer.reset();<br>
<br>
     // Atomically replace the existing file with the new one.<br>
-    auto EC = fs::rename(TempPath, FinalPath);<br>
-    sys::DontRemoveFileOnSignal(<wbr>TempPath);<br>
-    return errorCodeToError(EC);<br>
+    return Temp.keep(FinalPath);<br>
   }<br>
<br>
   ~OnDiskBuffer() override {<br>
     // Close the mapping before deleting the temp file, so that the removal<br>
     // succeeds.<br>
     Buffer.reset();<br>
-    fs::remove(TempPath);<br>
   }<br>
<br>
 private:<br>
   std::unique_ptr<fs::mapped_<wbr>file_region> Buffer;<br>
-  std::string TempPath;<br>
+  fs::TempFile Temp;<br>
 };<br>
<br>
 // A FileOutputBuffer which keeps data in memory and writes to the final<br>
@@ -110,13 +107,13 @@ createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {<br>
<br>
 static Expected<std::unique_ptr<<wbr>OnDiskBuffer>><br>
 createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {<br>
-  // Create new file in same directory but with random name.<br>
-  SmallString<128> TempPath;<br>
-  int FD;<br>
-  if (auto EC = fs::createUniqueFile(Path + ".tmp%%%%%%%", FD, TempPath, Mode))<br>
-    return errorCodeToError(EC);<br>
-<br>
-  sys::RemoveFileOnSignal(<wbr>TempPath);<br>
+  StringRef Directory = path::parent_path(Path);<br>
+  if (Directory.empty())<br>
+    Directory = ".";<br>
+  Expected<fs::TempFile> FileOrErr = fs::createUniqueFile(<wbr>Directory, Mode);<br>
+  if (!FileOrErr)<br>
+    return FileOrErr.takeError();<br>
+  fs::TempFile File = std::move(*FileOrErr);<br>
<br>
 #ifndef LLVM_ON_WIN32<br>
   // On Windows, CreateFileMapping (the mmap function on Windows)<br>
@@ -124,18 +121,18 @@ createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {<br>
   // extend the file beforehand. _chsize (ftruncate on Windows) is<br>
   // pretty slow just like it writes specified amount of bytes,<br>
   // so we should avoid calling that function.<br>
-  if (auto EC = fs::resize_file(FD, Size))<br>
+  if (auto EC = fs::resize_file(File.FD, Size))<br>
     return errorCodeToError(EC);<br>
 #endif<br>
<br>
   // Mmap it.<br>
   std::error_code EC;<br>
   auto MappedFile = llvm::make_unique<fs::mapped_<wbr>file_region>(<br>
-      FD, fs::mapped_file_region::<wbr>readwrite, Size, 0, EC);<br>
-  close(FD);<br>
+      File.FD, fs::mapped_file_region::<wbr>readwrite, Size, 0, EC);<br>
   if (EC)<br>
     return errorCodeToError(EC);<br>
-  return llvm::make_unique<<wbr>OnDiskBuffer>(Path, TempPath, std::move(MappedFile));<br>
+  return llvm::make_unique<<wbr>OnDiskBuffer>(Path, std::move(File),<br>
+                                         std::move(MappedFile));<br>
 }<br>
<br>
 // Create an instance of FileOutputBuffer.<br>
diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp<br>
index 9692acb5283..d3a356cecde 100644<br>
--- a/lib/Support/Path.cpp<br>
+++ b/lib/Support/Path.cpp<br>
@@ -18,8 +18,12 @@<br>
 #include "llvm/Support/ErrorHandling.h"<br>
 #include "llvm/Support/FileSystem.h"<br>
 #include "llvm/Support/Process.h"<br>
+#include "llvm/Support/Signals.h"<br>
 #include <cctype><br>
 #include <cstring><br>
+#include <fcntl.h><br>
+#include <sys/stat.h><br>
+#include <sys/types.h><br>
<br>
 #if !defined(_MSC_VER) && !defined(__MINGW32__)<br>
 #include <unistd.h><br>
@@ -759,6 +763,73 @@ std::error_code createUniqueFile(const Twine &Model,<br>
   return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name);<br>
 }<br>
<br>
+TempFile::TempFile(StringRef Name, int FD) : TmpName(Name), FD(FD) {}<br>
+TempFile::TempFile(TempFile &&Other) {<br>
+  TmpName = std::move(Other.TmpName);<br>
+  FD = Other.FD;<br>
+  Other.FD = -1;<br>
+}<br>
+<br>
+TempFile::~TempFile() {<br>
+  // FIXME: error handling. Move this to a discard method?<br>
+  if (FD != -1)<br>
+    close(FD);<br>
+  if (!TmpName.empty())<br>
+    fs::remove(TmpName);<br>
+  sys::DontRemoveFileOnSignal(<wbr>TmpName);<br>
+}<br>
+<br>
+Error TempFile::keep(const Twine &Name) {<br>
+  if (TmpName.empty()) {<br>
+    // We can't do it atomically.<br>
+    // FIXME: should we loop?<br>
+    if (std::error_code EC = fs::remove(Name))<br>
+      return errorCodeToError(EC);<br>
+    SmallString<128> Storage;<br>
+    StringRef NameP = Name.<wbr>toNullTerminatedStringRef(<wbr>Storage);<br>
+    std::string ProcFD = "/proc/self/fd/" + std::to_string(FD);<br>
+    if (linkat(AT_FDCWD, ProcFD.c_str(), AT_FDCWD, NameP.begin(),<br>
+               AT_SYMLINK_FOLLOW) == -1) {<br>
+      std::error_code EC(errno, std::generic_category());<br>
+      return errorCodeToError(EC);<br>
+    }<br>
+  } else {<br>
+    if (std::error_code EC = fs::rename(TmpName, Name))<br>
+      return errorCodeToError(EC);<br>
+    sys::DontRemoveFileOnSignal(<wbr>TmpName);<br>
+    TmpName = "";<br>
+  }<br>
+<br>
+  if (close(FD) == -1) {<br>
+    std::error_code EC(errno, std::generic_category());<br>
+    return errorCodeToError(EC);<br>
+  }<br>
+  FD = -1;<br>
+<br>
+  return Error::success();<br>
+}<br>
+<br>
+Expected<TempFile> createUniqueFile(const Twine &Directory, unsigned Mode) {<br>
+#if 1<br>
+  SmallString<128> Storage;<br>
+  StringRef P = Directory.<wbr>toNullTerminatedStringRef(<wbr>Storage);<br>
+  int FD = open(P.begin(), O_RDWR | O_TMPFILE | O_CLOEXEC, Mode);<br>
+  if (FD == -1) {<br>
+    std::error_code EC(errno, std::generic_category());<br>
+    return errorCodeToError(EC);<br>
+  }<br>
+  return TempFile("", FD);<br>
+#else<br>
+  int FD;<br>
+  SmallString<128> ResultPath;<br>
+  if (std::error_code EC =<br>
+          createUniqueFile(Directory + "/tmp%%%%%%%", FD, ResultPath, Mode))<br>
+    return errorCodeToError(EC);<br>
+  sys::RemoveFileOnSignal(<wbr>ResultPath);<br>
+  return TempFile(ResultPath, FD);<br>
+#endif<br>
+}<br>
+<br>
 static std::error_code<br>
 createTemporaryFile(const Twine &Model, int &ResultFD,<br>
                     llvm::SmallVectorImpl<char> &ResultPath, FSEntity Type) {<br>
diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc<br>
index 2ecb97316c8..3099b75a1b8 100644<br>
--- a/lib/Support/Unix/Path.inc<br>
+++ b/lib/Support/Unix/Path.inc<br>
@@ -779,6 +779,8 @@ std::error_code openFileForWrite(const Twine &Name, int &ResultFD,<br>
<br>
   int OpenFlags = O_CREAT;<br>
<br>
+  // add O_TMPFILE?<br>
+<br>
 #ifdef O_CLOEXEC<br>
   OpenFlags |= O_CLOEXEC;<br>
 #endif<br>
<br><br>
Cheers,<br>
Rafael<br>
<br>______________________________<wbr>_________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
<br></blockquote></div><br></div>