Changed createTemporaryFile without FD to actually create a file.

[ilya-biryukov]

Summary

This commit changes semantics of createUniqueFile and
createTemporaryFile variants that do not return file descriptors.
Previously they only checked if files exist, therefore being subject
to race conditions. Now they will create an empty file to avoid them.

Functions that do not create a file are now called
getPotentiallyUniqueTempFileName and getPotentiallyUniqueFileName.

[ilya-biryukov]

One test in clang repo breaks after this change, relevant fix is here: https://reviews.llvm.org/D36828

[klimek]

Is this still pending review?

[ilya-biryukov]

In D36827#880079, @klimek wrote:

Is this still pending review?

Yes.

[klimek]

Looks good. Sorry for the delay in review.

[ilya-biryukov]

In D36827#917666, @klimek wrote:

Looks good. Sorry for the delay in review.

No problem. Will make sure it still works and land it.

[JDevlieghere]

Rafael made some changes to have dsymutil use the new TempFile class in r318534 so I think it's no longer relevant there. Is this used anywhere else?

[ilya-biryukov]

In D36827#954038, @JDevlieghere wrote:

Rafael made some changes to have dsymutil use the new TempFile class in r318534 so I think it's no longer relevant there. Is this used anywhere else?

I haven't looked into that yet. Will let you know when I get to it. There was one more usage in ToolingTest.cpp, D36828 fixes it.
Good to know that dsymutil does not require any changes.

[JDevlieghere]

Hey Ilya, is this blocked by something or has this been landed already?

[ilya-biryukov]

In D36827#1041558, @JDevlieghere wrote:

Hey Ilya, is this blocked by something or has this been landed already?

Not yet landed, will have to rebase it and fix merge conflicts.
Will try to do that today.

[ilya-biryukov]

• Rebased onto head

[JDevlieghere]

Thanks!

[dstenb]

Hi! We have encountered a regression where clang-tidy leaves behind temporary files after this change. I wrote a PR for that: https://bugs.llvm.org/show_bug.cgi?id=37091.

include/llvm/Support/FileSystem.h

 /// @param ResultFD Set to the opened file's file descriptor.
 /// @param ResultPath Set to the opened file's absolute path.
 /// @returns errc::success if Result{FD,Path} have been successfully set,
 ///          otherwise a platform-specific error_code.
 std::error_code createUniqueFile(const Twine &Model, int &ResultFD,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode = all_read | all_write);
 
-/// @brief Simpler version for clients that don't want an open file.
+/// @brief Simpler version for clients that don't want an open file. An empty
+/// file will still be created.
 std::error_code createUniqueFile(const Twine &Model,
-                                 SmallVectorImpl<char> &ResultPath);
+                                 SmallVectorImpl<char> &ResultPath,
+                                 unsigned Mode = all_read | all_write);
 
 /// @brief Create a file in the system temporary directory.
 ///
 /// The filename is of the form prefix-random_chars.suffix. Since the directory
 /// is not know to the caller, Prefix and Suffix cannot have path separators.
 /// The files are created with mode 0600.
 ///
 /// This should be used for things like a temporary .s that is removed after
 /// running the assembler.
 std::error_code createTemporaryFile(const Twine &Prefix, StringRef Suffix,
                                     int &ResultFD,
                                     SmallVectorImpl<char> &ResultPath);
 
-/// @brief Simpler version for clients that don't want an open file.
+/// @brief Simpler version for clients that don't want an open file. An empty
+/// file will still be created.
 std::error_code createTemporaryFile(const Twine &Prefix, StringRef Suffix,
                                     SmallVectorImpl<char> &ResultPath);
 
 std::error_code createUniqueDirectory(const Twine &Prefix,
                                       SmallVectorImpl<char> &ResultPath);
 
+/// @brief Get a unique name, not currently exisiting in the filesystem. Subject
+/// to race conditions, prefer to use createUniqueFile instead.
+///
+/// Similar to createUniqueFile, but instead of creating a file only
+/// checks if it exists. This function is subject to race conditions, if you
+/// want to use the returned name to actually create a file, use
+/// createUniqueFile instead.
+std::error_code getPotentiallyUniqueFileName(const Twine &Model,
+                                             SmallVectorImpl<char> &ResultPath);
+
+/// @brief Get a unique temporary file name, not currently exisiting in the
+/// filesystem. Subject to race conditions, prefer to use createTemporaryFile
+/// instead.
+///
+/// Similar to createTemporaryFile, but instead of creating a file only
+/// checks if it exists. This function is subject to race conditions, if you
+/// want to use the returned name to actually create a file, use
+/// createTemporaryFile instead.
+std::error_code
+getPotentiallyUniqueTempFileName(const Twine &Prefix, StringRef Suffix,
+                                 SmallVectorImpl<char> &ResultPath);
+
 enum OpenFlags : unsigned {
   F_None = 0,
 
   /// F_Excl - When opening a file, this flag makes raw_fd_ostream
   /// report an error if the file already exists.
   F_Excl = 1,
 
   /// F_Append - When opening a file, if it already exists append to the

lib/Support/Path.cpp

 
 std::error_code createUniqueFile(const Twine &Model, int &ResultFd,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode) {
   return createUniqueEntity(Model, ResultFd, ResultPath, false, Mode, FS_File);
 }
 
 std::error_code createUniqueFile(const Twine &Model,
-                                 SmallVectorImpl<char> &ResultPath) {
-  int Dummy;
-  return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name);
+                                 SmallVectorImpl<char> &ResultPath,
+                                 unsigned Mode) {
+  int FD;
+  auto EC = createUniqueFile(Model, FD, ResultPath, Mode);
+  if (EC)
+    return EC;
+  // FD is only needed to avoid race conditions. Close it right away.
+  close(FD);
+  return EC;
 }
 
 static std::error_code
 createTemporaryFile(const Twine &Model, int &ResultFD,
                     llvm::SmallVectorImpl<char> &ResultPath, FSEntity Type) {
   SmallString<128> Storage;
   StringRef P = Model.toNullTerminatedStringRef(Storage);
   assert(P.find_first_of(separators(Style::native)) == StringRef::npos &&

 std::error_code createTemporaryFile(const Twine &Prefix, StringRef Suffix,
                                     int &ResultFD,
                                     SmallVectorImpl<char> &ResultPath) {
   return createTemporaryFile(Prefix, Suffix, ResultFD, ResultPath, FS_File);
 }
 
 std::error_code createTemporaryFile(const Twine &Prefix, StringRef Suffix,
                                     SmallVectorImpl<char> &ResultPath) {
-  int Dummy;
-  return createTemporaryFile(Prefix, Suffix, Dummy, ResultPath, FS_Name);
+  int FD;
+  auto EC = createTemporaryFile(Prefix, Suffix, FD, ResultPath);
+  if (EC)
+    return EC;
+  // FD is only needed to avoid race conditions. Close it right away.
+  close(FD);
+  return EC;
 }
 
 
 // This is a mkdtemp with a different pattern. We use createUniqueEntity mostly
 // for consistency. We should try using mkdtemp.
 std::error_code createUniqueDirectory(const Twine &Prefix,
                                       SmallVectorImpl<char> &ResultPath) {
   int Dummy;
-  return createUniqueEntity(Prefix + "-%%%%%%", Dummy, ResultPath,
-                            true, 0, FS_Dir);
+  return createUniqueEntity(Prefix + "-%%%%%%", Dummy, ResultPath, true, 0,
+                            FS_Dir);
+}
+
+std::error_code
+getPotentiallyUniqueFileName(const Twine &Model,
+                             SmallVectorImpl<char> &ResultPath) {
+  int Dummy;
+  return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name);
+}
+
+std::error_code
+getPotentiallyUniqueTempFileName(const Twine &Prefix, StringRef Suffix,
+                                 SmallVectorImpl<char> &ResultPath) {
+  int Dummy;
+  return createTemporaryFile(Prefix, Suffix, Dummy, ResultPath, FS_Name);
 }
 
 static std::error_code make_absolute(const Twine &current_directory,
                                      SmallVectorImpl<char> &path,
                                      bool use_current_directory) {
   StringRef p(path.data(), path.size());
 
   bool rootDirectory = path::has_root_directory(p);

tools/dsymutil/dsymutil.cpp

llvm::errs() << "error: cannot create directory " << Bundle << ": "
                  << EC.message() << "\n";
     return false;
   }
   return true;
 }
 
 static std::error_code getUniqueFile(const llvm::Twine &Model, int &ResultFD,
                                      llvm::SmallVectorImpl<char> &ResultPath) {
-  // If in NoOutput mode, use the createUniqueFile variant that
+  // If in NoOutput mode, use the getPotentiallyUniqueFileName that
   // doesn't open the file but still generates a somewhat unique
   // name. In the real usage scenario, we'll want to ensure that the
   // file is trully unique, and creating it is the only way to achieve
   // that.
   if (NoOutput)
-    return llvm::sys::fs::createUniqueFile(Model, ResultPath);
+    return llvm::sys::fs::getPotentiallyUniqueFileName(Model, ResultPath);
   return llvm::sys::fs::createUniqueFile(Model, ResultFD, ResultPath);
 }
 
 static std::string getOutputFileName(llvm::StringRef InputFile,
                                      bool TempFile = false) {
   if (TempFile) {
     llvm::SmallString<128> TmpFile;
     llvm::sys::path::system_temp_directory(true, TmpFile);