<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Nov 10, 2017 at 6:13 AM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</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></blockquote><div><br></div><div>I often execute lld in a chroot environment in which /proc is not mounted, and I expect lld would work just fine in that environment. So the presence of /proc should be considered optional even on Linux.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote><div><br></div><div>I think you can avoid unlink+linkat. You can instead give a temporary file name to an unnamed file and then rename the temporary file the destination file. There's a small chance between linkat and rename, but that race condition is probably better than unlink+linkat.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote></div><br></div></div>