hokein created this revision. hokein added a reviewer: avl. Herald added subscribers: ormris, kadircet, arphaman, steven_wu, hiraditya. Herald added a project: All. hokein requested review of this revision. Herald added projects: clang, LLDB, LLVM, clang-tools-extra. Herald added subscribers: llvm-commits, lldb-commits.
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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D153740 Files: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp clang/lib/Frontend/ASTUnit.cpp clang/lib/Serialization/GlobalModuleIndex.cpp lldb/tools/lldb-server/lldb-platform.cpp llvm/include/llvm/Support/FileUtilities.h llvm/lib/LTO/ThinLTOCodeGenerator.cpp llvm/lib/Support/FileUtilities.cpp llvm/unittests/Support/CMakeLists.txt llvm/unittests/Support/FileUtilitiesTest.cpp
Index: llvm/unittests/Support/FileUtilitiesTest.cpp =================================================================== --- llvm/unittests/Support/FileUtilitiesTest.cpp +++ /dev/null @@ -1,54 +0,0 @@ -//===- 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 Index: llvm/unittests/Support/CMakeLists.txt =================================================================== --- llvm/unittests/Support/CMakeLists.txt +++ llvm/unittests/Support/CMakeLists.txt @@ -41,7 +41,6 @@ ExtensibleRTTITest.cpp FileCollectorTest.cpp FileOutputBufferTest.cpp - FileUtilitiesTest.cpp FormatVariadicTest.cpp FSUniqueIDTest.cpp GlobPatternTest.cpp Index: llvm/lib/Support/FileUtilities.cpp =================================================================== --- llvm/lib/Support/FileUtilities.cpp +++ llvm/lib/Support/FileUtilities.cpp @@ -267,64 +267,6 @@ 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; @@ -389,5 +331,3 @@ return Error::success(); } - -char llvm::AtomicFileWriteError::ID; Index: llvm/lib/LTO/ThinLTOCodeGenerator.cpp =================================================================== --- llvm/lib/LTO/ThinLTOCodeGenerator.cpp +++ llvm/lib/LTO/ThinLTOCodeGenerator.cpp @@ -46,12 +46,14 @@ #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" @@ -415,28 +417,14 @@ 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())); } } }; Index: llvm/include/llvm/Support/FileUtilities.h =================================================================== --- llvm/include/llvm/Support/FileUtilities.h +++ llvm/include/llvm/Support/FileUtilities.h @@ -76,41 +76,6 @@ 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. Index: lldb/tools/lldb-server/lldb-platform.cpp =================================================================== --- lldb/tools/lldb-server/lldb-platform.cpp +++ lldb/tools/lldb-server/lldb-platform.cpp @@ -103,35 +103,12 @@ 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()); } Index: clang/lib/Serialization/GlobalModuleIndex.cpp =================================================================== --- clang/lib/Serialization/GlobalModuleIndex.cpp +++ clang/lib/Serialization/GlobalModuleIndex.cpp @@ -31,6 +31,7 @@ #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; @@ -907,8 +908,10 @@ "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 { Index: clang/lib/Frontend/ASTUnit.cpp =================================================================== --- clang/lib/Frontend/ASTUnit.cpp +++ clang/lib/Frontend/ASTUnit.cpp @@ -2313,16 +2313,11 @@ 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()) Index: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp =================================================================== --- clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp +++ clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp @@ -14,9 +14,9 @@ #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> @@ -67,11 +67,10 @@ 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) { + OS << Shard; + return llvm::Error::success(); + }); } };
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
