[llvm] r338216 - [dsymutil] Simplify temporary file handling.
Jonas Devlieghere via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 29 07:56:16 PDT 2018
Author: jdevlieghere
Date: Sun Jul 29 07:56:15 2018
New Revision: 338216
URL: http://llvm.org/viewvc/llvm-project?rev=338216&view=rev
Log:
[dsymutil] Simplify temporary file handling.
Dsymutil's update functionality was broken on Windows because we tried
to rename a file while we're holding open handles to that file. TempFile
provides a solution for this through its keep(Twine) method. This patch
changes dsymutil to make use of that functionality.
Differential revision: https://reviews.llvm.org/D49860
Modified:
llvm/trunk/lib/Support/Path.cpp
llvm/trunk/lib/Support/Windows/Path.inc
llvm/trunk/test/tools/dsymutil/X86/accelerator.test
llvm/trunk/test/tools/dsymutil/X86/update-one-CU.test
llvm/trunk/test/tools/dsymutil/X86/update.test
llvm/trunk/tools/dsymutil/MachOUtils.cpp
llvm/trunk/tools/dsymutil/MachOUtils.h
llvm/trunk/tools/dsymutil/dsymutil.cpp
Modified: llvm/trunk/lib/Support/Path.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Path.cpp (original)
+++ llvm/trunk/lib/Support/Path.cpp Sun Jul 29 07:56:15 2018
@@ -1157,9 +1157,13 @@ Error TempFile::keep(const Twine &Name)
setDeleteDisposition(H, true);
#else
std::error_code RenameEC = fs::rename(TmpName, Name);
- // If we can't rename, discard the temporary file.
- if (RenameEC)
- remove(TmpName);
+ if (RenameEC) {
+ // If we can't rename, try to copy to work around cross-device link issues.
+ RenameEC = sys::fs::copy_file(TmpName, Name);
+ // If we can't rename or copy, discard the temporary file.
+ if (RenameEC)
+ remove(TmpName);
+ }
sys::DontRemoveFileOnSignal(TmpName);
#endif
Modified: llvm/trunk/lib/Support/Windows/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Path.inc (original)
+++ llvm/trunk/lib/Support/Windows/Path.inc Sun Jul 29 07:56:15 2018
@@ -450,7 +450,7 @@ static std::error_code rename_handle(HAN
if (std::error_code EC2 = realPathFromHandle(FromHandle, WideFrom))
return EC2;
if (::MoveFileExW(WideFrom.begin(), WideTo.begin(),
- MOVEFILE_REPLACE_EXISTING))
+ MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED))
return std::error_code();
return mapWindowsError(GetLastError());
}
Modified: llvm/trunk/test/tools/dsymutil/X86/accelerator.test
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/X86/accelerator.test?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/test/tools/dsymutil/X86/accelerator.test (original)
+++ llvm/trunk/test/tools/dsymutil/X86/accelerator.test Sun Jul 29 07:56:15 2018
@@ -1,7 +1,3 @@
-UNSUPPORTED: system-windows
-Windows does not like renaming files that have open handles to them. We
-need to use TempFile::keep to move them in a portable way.
-
RUN: dsymutil -accelerator=Dwarf -oso-prepend-path=%p/.. %p/../Inputs/basic.macho.x86_64 -o %t.dwarf.dSYM
RUN: dsymutil -accelerator=Apple -oso-prepend-path=%p/.. %p/../Inputs/basic.macho.x86_64 -o %t.apple.dSYM
Modified: llvm/trunk/test/tools/dsymutil/X86/update-one-CU.test
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/X86/update-one-CU.test?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/test/tools/dsymutil/X86/update-one-CU.test (original)
+++ llvm/trunk/test/tools/dsymutil/X86/update-one-CU.test Sun Jul 29 07:56:15 2018
@@ -1,7 +1,3 @@
-UNSUPPORTED: system-windows
-Windows does not like renaming files that have open handles to them. We
-need to use TempFile::keep to move them in a portable way.
-
RUN: dsymutil -oso-prepend-path=%p/.. %p/../Inputs/objc.macho.x86_64 -o %t.dSYM
RUN: dsymutil -update %t.dSYM
RUN: llvm-dwarfdump -apple-types -apple-objc %t.dSYM | FileCheck %s
Modified: llvm/trunk/test/tools/dsymutil/X86/update.test
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/X86/update.test?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/test/tools/dsymutil/X86/update.test (original)
+++ llvm/trunk/test/tools/dsymutil/X86/update.test Sun Jul 29 07:56:15 2018
@@ -1,7 +1,3 @@
-UNSUPPORTED: system-windows
-Windows does not like renaming files that have open handles to them. We
-need to use TempFile::keep to move them in a portable way.
-
RUN: rm -rf %t.dir
RUN: mkdir -p %t.dir
RUN: cat %p/../Inputs/basic.macho.x86_64 > %t.dir/basic
Modified: llvm/trunk/tools/dsymutil/MachOUtils.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/MachOUtils.cpp?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/tools/dsymutil/MachOUtils.cpp (original)
+++ llvm/trunk/tools/dsymutil/MachOUtils.cpp Sun Jul 29 07:56:15 2018
@@ -27,6 +27,27 @@ namespace llvm {
namespace dsymutil {
namespace MachOUtils {
+llvm::Error ArchAndFile::createTempFile() {
+ llvm::SmallString<128> TmpModel;
+ llvm::sys::path::system_temp_directory(true, TmpModel);
+ llvm::sys::path::append(TmpModel, "dsym.tmp%%%%%.dwarf");
+ Expected<sys::fs::TempFile> T = sys::fs::TempFile::create(TmpModel);
+
+ if (!T)
+ return T.takeError();
+
+ File = llvm::Optional<sys::fs::TempFile>(std::move(*T));
+ return Error::success();
+}
+
+llvm::StringRef ArchAndFile::path() const { return File->TmpName; }
+
+ArchAndFile::~ArchAndFile() {
+ if (File)
+ if (auto E = File->discard())
+ llvm::consumeError(std::move(E));
+}
+
std::string getArchName(StringRef Arch) {
if (Arch.startswith("thumb"))
return (llvm::Twine("arm") + Arch.drop_front(5)).str();
@@ -53,21 +74,16 @@ static bool runLipo(StringRef SDKPath, S
return true;
}
-bool generateUniversalBinary(SmallVectorImpl<ArchAndFilename> &ArchFiles,
+bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
StringRef OutputFileName,
const LinkOptions &Options, StringRef SDKPath) {
- // No need to merge one file into a universal fat binary. First, try
- // to move it (rename) to the final location. If that fails because
- // of cross-device link issues then copy and delete.
+ // No need to merge one file into a universal fat binary.
if (ArchFiles.size() == 1) {
- StringRef From(ArchFiles.front().Path);
- if (sys::fs::rename(From, OutputFileName)) {
- if (std::error_code EC = sys::fs::copy_file(From, OutputFileName)) {
- WithColor::error() << "while copying " << From << " to "
- << OutputFileName << ": " << EC.message() << "\n";
- return false;
- }
- sys::fs::remove(From);
+ if (auto E = ArchFiles.front().File->keep(OutputFileName)) {
+ WithColor::error() << "while keeping " << ArchFiles.front().path()
+ << " as " << OutputFileName << ": "
+ << toString(std::move(E)) << "\n";
+ return false;
}
return true;
}
@@ -77,7 +93,7 @@ bool generateUniversalBinary(SmallVector
Args.push_back("-create");
for (auto &Thin : ArchFiles)
- Args.push_back(Thin.Path);
+ Args.push_back(Thin.path());
// Align segments to match dsymutil-classic alignment
for (auto &Thin : ArchFiles) {
Modified: llvm/trunk/tools/dsymutil/MachOUtils.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/MachOUtils.h?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/tools/dsymutil/MachOUtils.h (original)
+++ llvm/trunk/tools/dsymutil/MachOUtils.h Sun Jul 29 07:56:15 2018
@@ -10,6 +10,7 @@
#define LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
#include <string>
namespace llvm {
@@ -20,12 +21,20 @@ class DebugMap;
struct LinkOptions;
namespace MachOUtils {
-struct ArchAndFilename {
- std::string Arch, Path;
- ArchAndFilename(StringRef Arch, StringRef Path) : Arch(Arch), Path(Path) {}
+struct ArchAndFile {
+ std::string Arch;
+ // Optional because TempFile has no default constructor.
+ Optional<llvm::sys::fs::TempFile> File;
+
+ llvm::Error createTempFile();
+ llvm::StringRef path() const;
+
+ ArchAndFile(StringRef Arch) : Arch(Arch) {}
+ ArchAndFile(ArchAndFile &&A) = default;
+ ~ArchAndFile();
};
-bool generateUniversalBinary(SmallVectorImpl<ArchAndFilename> &ArchFiles,
+bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
StringRef OutputFileName, const LinkOptions &,
StringRef SDKPath);
Modified: llvm/trunk/tools/dsymutil/dsymutil.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/dsymutil.cpp?rev=338216&r1=338215&r2=338216&view=diff
==============================================================================
--- llvm/trunk/tools/dsymutil/dsymutil.cpp (original)
+++ llvm/trunk/tools/dsymutil/dsymutil.cpp Sun Jul 29 07:56:15 2018
@@ -313,13 +313,6 @@ static std::string getOutputFileName(llv
return BundleDir.str();
}
-static Expected<sys::fs::TempFile> createTempFile() {
- llvm::SmallString<128> TmpModel;
- llvm::sys::path::system_temp_directory(true, TmpModel);
- llvm::sys::path::append(TmpModel, "dsym.tmp%%%%%.dwarf");
- return sys::fs::TempFile::create(TmpModel);
-}
-
/// Parses the command line options into the LinkOptions struct and performs
/// some sanity checking. Returns an error in case the latter fails.
static Expected<LinkOptions> getOptions() {
@@ -400,18 +393,6 @@ static Expected<std::vector<std::string>
return Inputs;
}
-namespace {
-struct TempFileVector {
- std::vector<sys::fs::TempFile> Files;
- ~TempFileVector() {
- for (sys::fs::TempFile &Tmp : Files) {
- if (Error E = Tmp.discard())
- errs() << toString(std::move(E));
- }
- }
-};
-} // namespace
-
int main(int argc, char **argv) {
InitLLVM X(argc, argv);
@@ -523,8 +504,7 @@ int main(int argc, char **argv) {
!DumpDebugMap && (OutputFileOpt != "-") &&
(DebugMapPtrsOrErr->size() != 1 || OptionsOrErr->Update);
- llvm::SmallVector<MachOUtils::ArchAndFilename, 4> TempFiles;
- TempFileVector TempFileStore;
+ llvm::SmallVector<MachOUtils::ArchAndFile, 4> TempFiles;
std::atomic_char AllOK(1);
for (auto &Map : *DebugMapPtrsOrErr) {
if (Verbose || DumpDebugMap)
@@ -543,16 +523,18 @@ int main(int argc, char **argv) {
std::shared_ptr<raw_fd_ostream> OS;
std::string OutputFile = getOutputFileName(InputFile);
if (NeedsTempFiles) {
- Expected<sys::fs::TempFile> T = createTempFile();
- if (!T) {
- errs() << toString(T.takeError());
+ TempFiles.emplace_back(Map->getTriple().getArchName().str());
+
+ auto E = TempFiles.back().createTempFile();
+ if (E) {
+ errs() << toString(std::move(E));
return 1;
}
- OS = std::make_shared<raw_fd_ostream>(T->FD, /*shouldClose*/ false);
- OutputFile = T->TmpName;
- TempFileStore.Files.push_back(std::move(*T));
- TempFiles.emplace_back(Map->getTriple().getArchName().str(),
- OutputFile);
+
+ auto &TempFile = *(TempFiles.back().File);
+ OS = std::make_shared<raw_fd_ostream>(TempFile.FD,
+ /*shouldClose*/ false);
+ OutputFile = TempFile.TmpName;
} else {
std::error_code EC;
OS = std::make_shared<raw_fd_ostream>(NoOutput ? "-" : OutputFile, EC,
More information about the llvm-commits
mailing list