[llvm][Support] Deprecate llvm::writeFileAtomically API

[hokein]

Summary

The new llvm::writeToOutput API does the same thing, and we're in favor
of it.

This patch migrates 4 exiting usages and removes the llvm::writeFileAtomically API.

[hokein]

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

[avl]

added @jkorous who originally added llvm::writeFileAtomically.

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

I think it is OK to leave only one API (f.e. llvm::writeToOutput) and remove another. It would probably be better to split this patch to lldb, thinlto, clang and removal parts.

lldb/tools/lldb-server/lldb-platform.cpp

107

the call to llvm::writeToOutput is probably missed here.

[hokein]

In D153740#4448408, @avl wrote:

added @jkorous who originally added llvm::writeFileAtomically.

Let me know what you think about it -- I considered keeping the llvm::writeFileAtomically and migrating its underlying implementation to llvm::writeToOutput, but it doesn't seem to worth, there are only 4 in-tree usages of this API, I think it is probably better just remove it.

I think it is OK to leave only one API (f.e. llvm::writeToOutput) and remove another. It would probably be better to split this patch to lldb, thinlto, clang and removal parts.

Sounds good. I will split it once https://reviews.llvm.org/D153652 is landed.

[hokein]

Restrict the patch to only remove the writeFileAtomically API

[hokein]

Now this patch only contains the removal part of the API (I have cleaned all usages of writeFileAtomically API, except a remaining one in lldb https://reviews.llvm.org/D154329).

lldb/tools/lldb-server/lldb-platform.cpp

107

oops, good catch, the handleError should be writeToOutput.

[avl]

this LGTM, assuming D154329 is landed. Thank you!

clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp

 //== BackgroundIndexStorage.cpp - Provide caching support to BackgroundIndex ==/
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
 #include "index/Background.h"
 #include "support/Logger.h"
 #include "support/Path.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include <functional>
 #include <optional>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 std::string getShardPathFromFilePath(llvm::StringRef ShardRoot,
else
       elog("Error while reading shard {0}: {1}", ShardIdentifier,
            I.takeError());
     return nullptr;
   }
 
   llvm::Error storeShard(llvm::StringRef ShardIdentifier,
                          IndexFileOut Shard) const override {
     auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
-    return llvm::writeFileAtomically(ShardPath + ".tmp.%%%%%%%%", ShardPath,
-                                     [&Shard](llvm::raw_ostream &OS) {
-                                       OS << Shard;
-                                       return llvm::Error::success();
-                                     });
+    return llvm::writeToOutput(ShardPath, [&Shard](llvm::raw_ostream &OS) {
   }
 };
 
 // Doesn't persist index shards anywhere (used when the CDB dir is unknown).
 // We could consider indexing into ~/.clangd/ or so instead.
 class NullStorage : public BackgroundIndexStorage {
 public:
   std::unique_ptr<IndexFileIn>

clang/lib/Frontend/ASTUnit.cpp

if (Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0])) {
     Act->EndSourceFile();
   }
 }
 
 bool ASTUnit::Save(StringRef File) {
   if (HadModuleLoaderFatalFailure)
     return true;
 
-  // Write to a temporary file and later rename it to the actual file, to avoid
-  // possible race conditions.
-  SmallString<128> TempPath;
-  TempPath = File;
-  TempPath += "-%%%%%%%%";
   // FIXME: Can we somehow regenerate the stat cache here, or do we need to
   // unconditionally create a stat cache when we parse the file?
 
-  if (llvm::Error Err = llvm::writeFileAtomically(
-          TempPath, File, [this](llvm::raw_ostream &Out) {
+  if (llvm::Error Err = llvm::writeToOutput(
+          File, [this](llvm::raw_ostream &Out) {
             return serialize(Out) ? llvm::make_error<llvm::StringError>(
                                         "ASTUnit serialization failed",
                                         llvm::inconvertibleErrorCode())
                                   : llvm::Error::success();
           })) {
     consumeError(std::move(Err));
     return true;
   }

clang/lib/Serialization/GlobalModuleIndex.cpp

 #include "llvm/Support/DJB.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/LockFileManager.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/OnDiskHashTable.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TimeProfiler.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cstdio>
 using namespace clang;
 using namespace serialization;
 
 //----------------------------------------------------------------------------//
 // Shared constants
 //----------------------------------------------------------------------------//
 namespace {
GlobalModuleIndex::writeIndex(FileManager &FileMgr,
   SmallString<16> OutputBuffer;
   {
     llvm::BitstreamWriter OutputStream(OutputBuffer);
     if (Builder.writeIndex(OutputStream))
       return llvm::createStringError(std::errc::io_error,
                                      "failed writing index");
   }
 
-  return llvm::writeFileAtomically((IndexPath + "-%%%%%%%%").str(), IndexPath,
-                                   OutputBuffer);
+  return llvm::writeToOutput(IndexPath, [&OutputBuffer](llvm::raw_ostream &OS) {
+    OS << OutputBuffer;
+    return llvm::Error::success();
+  });
 }
 
 namespace {
   class GlobalIndexIdentifierIterator : public IdentifierIterator {
     /// The current position within the identifier lookup table.
     IdentifierIndexTable::key_iterator Current;
 
     /// The end position within the identifier lookup table.

lldb/tools/lldb-server/lldb-platform.cpp

 static Status save_socket_id_to_file(const std::string &socket_id,
                                      const FileSpec &file_spec) {
   FileSpec temp_file_spec(file_spec.GetDirectory().GetStringRef());
   Status error(llvm::sys::fs::create_directory(temp_file_spec.GetPath()));
   if (error.Fail())
     return Status("Failed to create directory %s: %s",
                   temp_file_spec.GetPath().c_str(), error.AsCString());
 
-  llvm::SmallString<64> temp_file_path;
-  temp_file_spec.AppendPathComponent("port-file.%%%%%%");
-  temp_file_path = temp_file_spec.GetPath();
-
   Status status;
-  if (auto Err =
-          handleErrors(llvm::writeFileAtomically(
-                           temp_file_path, file_spec.GetPath(), socket_id),
-                       [&status, &file_spec](const AtomicFileWriteError &E) {
-                         std::string ErrorMsgBuffer;
-                         llvm::raw_string_ostream S(ErrorMsgBuffer);
-                         E.log(S);
-
-                         switch (E.Error) {
-                         case atomic_write_error::failed_to_create_uniq_file:
-                           status = Status("Failed to create temp file: %s",
-                                           ErrorMsgBuffer.c_str());
-                           break;
-                         case atomic_write_error::output_stream_error:
-                           status = Status("Failed to write to port file.");
-                           break;
-                         case atomic_write_error::failed_to_rename_temp_file:
-                           status = Status("Failed to rename file %s to %s: %s",
-                                           ErrorMsgBuffer.c_str(),
-                                           file_spec.GetPath().c_str(),
-                                           ErrorMsgBuffer.c_str());
-                           break;
-                         }
+  if (auto Err = handleErrors(file_spec.GetPath(),
+                              [&socket_id](llvm::raw_ostream &OS) {
+                                OS << socket_id;
+                                return llvm::Error::success();
                               })) {
     return Status("Failed to atomically write file %s",
                   file_spec.GetPath().c_str());
   }
   return status;
 }
 
 // main
 int main_platform(int argc, char *argv[]) {

llvm/include/llvm/Support/FileUtilities.h

void setFile(const Twine& filename, bool deleteIt = true) {
       DeleteIt = deleteIt;
     }
 
     /// releaseFile - Take ownership of the file away from the FileRemover so it
     /// will not be removed when the object is destroyed.
     void releaseFile() { DeleteIt = false; }
   };
 
-  enum class atomic_write_error {
-    failed_to_create_uniq_file = 0,
-    output_stream_error,
-    failed_to_rename_temp_file
-  };
-
-  class AtomicFileWriteError : public llvm::ErrorInfo<AtomicFileWriteError> {
-  public:
-    AtomicFileWriteError(atomic_write_error Error) : Error(Error) {}
-
-    void log(raw_ostream &OS) const override;
-
-    const atomic_write_error Error;
-    static char ID;
-
-  private:
-    // Users are not expected to use error_code.
-    std::error_code convertToErrorCode() const override {
-      return llvm::inconvertibleErrorCode();
-    }
-  };
-
-  // atomic_write_error + whatever the Writer can return
-
-  /// Creates a unique file with name according to the given \p TempPathModel,
-  /// writes content of \p Buffer to the file and renames it to \p FinalPath.
-  ///
-  /// \returns \c AtomicFileWriteError in case of error.
-  llvm::Error writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
-                                  StringRef Buffer);
-
-  llvm::Error
-  writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
-                      std::function<llvm::Error(llvm::raw_ostream &)> Writer);
-
   /// FilePermssionsApplier helps to copy permissions from an input file to
   /// an output one. It memorizes the status of the input file and can apply
   /// permissions and dates to the output file.
   class FilePermissionsApplier {
   public:
     static Expected<FilePermissionsApplier> create(StringRef InputFilename);
 
     /// Apply stored permissions to the \p OutputFilename.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp

 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Remarks/HotnessThresholdParser.h"
 #include "llvm/Support/CachePruning.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/FunctionAttrs.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
 #include "llvm/Transforms/IPO/Internalize.h"
 #include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
 
ErrorOr<std::unique_ptr<MemoryBuffer>> tryLoadingBuffer() {
     return MBOrErr;
   }
 
   // Cache the Produced object file
   void write(const MemoryBuffer &OutputBuffer) {
     if (EntryPath.empty())
       return;
 
-    // Write to a temporary to avoid race condition
-    SmallString<128> TempFilename;
-    SmallString<128> CachePath(EntryPath);
-    llvm::sys::path::remove_filename(CachePath);
-    sys::path::append(TempFilename, CachePath, "Thin-%%%%%%.tmp.o");
-
-    if (auto Err = handleErrors(
-            llvm::writeFileAtomically(TempFilename, EntryPath,
-                                      OutputBuffer.getBuffer()),
-            [](const llvm::AtomicFileWriteError &E) {
-              std::string ErrorMsgBuffer;
-              llvm::raw_string_ostream S(ErrorMsgBuffer);
-              E.log(S);
-
-              if (E.Error ==
-                  llvm::atomic_write_error::failed_to_create_uniq_file) {
-                errs() << "Error: " << ErrorMsgBuffer << "\n";
-                report_fatal_error("ThinLTO: Can't get a temporary file");
-              }
-            })) {
-      // FIXME
-      consumeError(std::move(Err));
+    if (auto Err = handleErrors(llvm::writeToOutput(
+            EntryPath, [&OutputBuffer](llvm::raw_ostream &OS) -> llvm::Error {
+              OS << OutputBuffer.getBuffer();
+              return llvm::Error::success();
+            }))) {
+      report_fatal_error(llvm::formatv("ThinLTO: Can't write file {0}: {1}",
+                                       EntryPath,
+                                       toString(std::move(Err)).c_str()));
     }
   }
 };
 
 static std::unique_ptr<MemoryBuffer>
 ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
                      StringMap<lto::InputFile *> &ModuleMap, TargetMachine &TM,
                      const FunctionImporter::ImportMapTy &ImportList,

llvm/lib/Support/FileUtilities.cpp

if (!CompareFailed && (!F1AtEnd || !F2AtEnd)) {
     // If we found the end, we succeeded.
     if (F1P < File1End || F2P < File2End)
       CompareFailed = true;
   }
 
   return CompareFailed;
 }
 
-void llvm::AtomicFileWriteError::log(raw_ostream &OS) const {
-  OS << "atomic_write_error: ";
-  switch (Error) {
-  case atomic_write_error::failed_to_create_uniq_file:
-    OS << "failed_to_create_uniq_file";
-    return;
-  case atomic_write_error::output_stream_error:
-    OS << "output_stream_error";
-    return;
-  case atomic_write_error::failed_to_rename_temp_file:
-    OS << "failed_to_rename_temp_file";
-    return;
-  }
-  llvm_unreachable("unknown atomic_write_error value in "
-                   "failed_to_rename_temp_file::log()");
-}
-
-llvm::Error llvm::writeFileAtomically(StringRef TempPathModel,
-                                      StringRef FinalPath, StringRef Buffer) {
-  return writeFileAtomically(TempPathModel, FinalPath,
-                             [&Buffer](llvm::raw_ostream &OS) {
-                               OS.write(Buffer.data(), Buffer.size());
-                               return llvm::Error::success();
-                             });
-}
-
-llvm::Error llvm::writeFileAtomically(
-    StringRef TempPathModel, StringRef FinalPath,
-    std::function<llvm::Error(llvm::raw_ostream &)> Writer) {
-  SmallString<128> GeneratedUniqPath;
-  int TempFD;
-  if (sys::fs::createUniqueFile(TempPathModel, TempFD, GeneratedUniqPath)) {
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::failed_to_create_uniq_file);
-  }
-  llvm::FileRemover RemoveTmpFileOnFail(GeneratedUniqPath);
-
-  raw_fd_ostream OS(TempFD, /*shouldClose=*/true);
-  if (llvm::Error Err = Writer(OS)) {
-    return Err;
-  }
-
-  OS.close();
-  if (OS.has_error()) {
-    OS.clear_error();
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::output_stream_error);
-  }
-
-  if (sys::fs::rename(/*from=*/GeneratedUniqPath, /*to=*/FinalPath)) {
-    return llvm::make_error<AtomicFileWriteError>(
-        atomic_write_error::failed_to_rename_temp_file);
-  }
-
-  RemoveTmpFileOnFail.releaseFile();
-  return Error::success();
-}
-
 Expected<FilePermissionsApplier>
 FilePermissionsApplier::create(StringRef InputFilename) {
   sys::fs::file_status Status;
 
   if (InputFilename != "-") {
     if (auto EC = sys::fs::status(InputFilename, Status))
       return createFileError(InputFilename, EC);
   } else {
#endif
       return createFileError(OutputFilename, EC);
   }
 
   if (std::error_code EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(OutputFilename, EC);
 
   return Error::success();
 }
-
-char llvm::AtomicFileWriteError::ID;

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   EndianTest.cpp
   ELFAttributeParserTest.cpp
   ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   ExtensibleRTTITest.cpp
   FileCollectorTest.cpp
   FileOutputBufferTest.cpp
-  FileUtilitiesTest.cpp
   FormatVariadicTest.cpp
   FSUniqueIDTest.cpp
   GlobPatternTest.cpp
   HashBuilderTest.cpp
   IndexedAccessorTest.cpp
   InstructionCostTest.cpp
   JSONTest.cpp
   KnownBitsTest.cpp

llvm/unittests/Support/FileUtilitiesTest.cpp

-//===- llvm/unittest/Support/FileUtilitiesTest.cpp - unit tests -----------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "llvm/Support/FileUtilities.h"
-#include "llvm/Support/Errc.h"
-#include "llvm/Support/ErrorHandling.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Testing/Support/SupportHelpers.h"
-#include "gtest/gtest.h"
-#include <fstream>
-
-using namespace llvm;
-using namespace llvm::sys;
-
-using llvm::unittest::TempDir;
-
-#define ASSERT_NO_ERROR(x)                                                     \
-  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
-    SmallString<128> MessageStorage;                                           \
-    raw_svector_ostream Message(MessageStorage);                               \
-    Message << #x ": did not return errc::success.\n"                          \
-            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
-            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
-    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
-  } else {                                                                     \
-  }
-
-namespace {
-TEST(writeFileAtomicallyTest, Test) {
-  // Create unique temporary directory for these tests
-  TempDir RootTestDirectory("writeFileAtomicallyTest", /*Unique*/ true);
-
-  SmallString<128> FinalTestfilePath(RootTestDirectory.path());
-  sys::path::append(FinalTestfilePath, "foo.txt");
-  const std::string TempUniqTestFileModel =
-      std::string(FinalTestfilePath) + "-%%%%%%%%";
-  const std::string TestfileContent = "fooFOOfoo";
-
-  llvm::Error Err = llvm::writeFileAtomically(TempUniqTestFileModel, FinalTestfilePath, TestfileContent);
-  ASSERT_FALSE(static_cast<bool>(Err));
-
-  std::ifstream FinalFileStream(std::string(FinalTestfilePath.str()));
-  std::string FinalFileContent;
-  FinalFileStream >> FinalFileContent;
-  ASSERT_EQ(FinalFileContent, TestfileContent);
-}
-} // anonymous namespace