diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp index a806da23ec50..098230290ed2 100644 --- a/llvm/lib/Support/Path.cpp +++ b/llvm/lib/Support/Path.cpp @@ -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 diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index f425d607af47..b64b013d7407 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -450,7 +450,7 @@ static std::error_code rename_handle(HANDLE FromHandle, const Twine &To) { 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()); } diff --git a/llvm/test/tools/dsymutil/X86/accelerator.test b/llvm/test/tools/dsymutil/X86/accelerator.test index 906b0e645cf4..96fc58ee5683 100644 --- a/llvm/test/tools/dsymutil/X86/accelerator.test +++ b/llvm/test/tools/dsymutil/X86/accelerator.test @@ -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 diff --git a/llvm/test/tools/dsymutil/X86/update-one-CU.test b/llvm/test/tools/dsymutil/X86/update-one-CU.test index 5d36ce7135f2..09f49ca89c48 100644 --- a/llvm/test/tools/dsymutil/X86/update-one-CU.test +++ b/llvm/test/tools/dsymutil/X86/update-one-CU.test @@ -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 diff --git a/llvm/test/tools/dsymutil/X86/update.test b/llvm/test/tools/dsymutil/X86/update.test index cbfff63d6a87..804091ab2943 100644 --- a/llvm/test/tools/dsymutil/X86/update.test +++ b/llvm/test/tools/dsymutil/X86/update.test @@ -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 diff --git a/llvm/tools/dsymutil/MachOUtils.cpp b/llvm/tools/dsymutil/MachOUtils.cpp index eda530b810c0..fc498cc49c19 100644 --- a/llvm/tools/dsymutil/MachOUtils.cpp +++ b/llvm/tools/dsymutil/MachOUtils.cpp @@ -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 T = sys::fs::TempFile::create(TmpModel); + + if (!T) + return T.takeError(); + + File = llvm::Optional(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, SmallVectorImpl &Args) { return true; } -bool generateUniversalBinary(SmallVectorImpl &ArchFiles, +bool generateUniversalBinary(SmallVectorImpl &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(SmallVectorImpl &ArchFiles, 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) { diff --git a/llvm/tools/dsymutil/MachOUtils.h b/llvm/tools/dsymutil/MachOUtils.h index 0db8ed1a1e31..a8be89e906b5 100644 --- a/llvm/tools/dsymutil/MachOUtils.h +++ b/llvm/tools/dsymutil/MachOUtils.h @@ -10,6 +10,7 @@ #define LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H #include "llvm/ADT/StringRef.h" +#include "llvm/Support/FileSystem.h" #include 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 File; + + llvm::Error createTempFile(); + llvm::StringRef path() const; + + ArchAndFile(StringRef Arch) : Arch(Arch) {} + ArchAndFile(ArchAndFile &&A) = default; + ~ArchAndFile(); }; -bool generateUniversalBinary(SmallVectorImpl &ArchFiles, +bool generateUniversalBinary(SmallVectorImpl &ArchFiles, StringRef OutputFileName, const LinkOptions &, StringRef SDKPath); diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp index fc447b30be98..c0e6d505941f 100644 --- a/llvm/tools/dsymutil/dsymutil.cpp +++ b/llvm/tools/dsymutil/dsymutil.cpp @@ -313,13 +313,6 @@ static std::string getOutputFileName(llvm::StringRef InputFile) { return BundleDir.str(); } -static Expected 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 getOptions() { @@ -400,18 +393,6 @@ static Expected> getInputs(bool DsymAsInput) { return Inputs; } -namespace { -struct TempFileVector { - std::vector 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 TempFiles; - TempFileVector TempFileStore; + llvm::SmallVector 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 OS; std::string OutputFile = getOutputFileName(InputFile); if (NeedsTempFiles) { - Expected 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(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(TempFile.FD, + /*shouldClose*/ false); + OutputFile = TempFile.TmpName; } else { std::error_code EC; OS = std::make_shared(NoOutput ? "-" : OutputFile, EC,