Skip to content

Commit ae1727e

Browse files
JDevlieghereJDevlieghere
committedJul 29, 2018
[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 llvm-svn: 338216
1 parent f52eeb1 commit ae1727e

File tree

8 files changed

+61
-62
lines changed

8 files changed

+61
-62
lines changed
 

‎llvm/lib/Support/Path.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,9 +1157,13 @@ Error TempFile::keep(const Twine &Name) {
11571157
setDeleteDisposition(H, true);
11581158
#else
11591159
std::error_code RenameEC = fs::rename(TmpName, Name);
1160-
// If we can't rename, discard the temporary file.
1161-
if (RenameEC)
1162-
remove(TmpName);
1160+
if (RenameEC) {
1161+
// If we can't rename, try to copy to work around cross-device link issues.
1162+
RenameEC = sys::fs::copy_file(TmpName, Name);
1163+
// If we can't rename or copy, discard the temporary file.
1164+
if (RenameEC)
1165+
remove(TmpName);
1166+
}
11631167
sys::DontRemoveFileOnSignal(TmpName);
11641168
#endif
11651169

‎llvm/lib/Support/Windows/Path.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,7 @@ static std::error_code rename_handle(HANDLE FromHandle, const Twine &To) {
450450
if (std::error_code EC2 = realPathFromHandle(FromHandle, WideFrom))
451451
return EC2;
452452
if (::MoveFileExW(WideFrom.begin(), WideTo.begin(),
453-
MOVEFILE_REPLACE_EXISTING))
453+
MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED))
454454
return std::error_code();
455455
return mapWindowsError(GetLastError());
456456
}

‎llvm/test/tools/dsymutil/X86/accelerator.test

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
UNSUPPORTED: system-windows
2-
Windows does not like renaming files that have open handles to them. We
3-
need to use TempFile::keep to move them in a portable way.
4-
51
RUN: dsymutil -accelerator=Dwarf -oso-prepend-path=%p/.. %p/../Inputs/basic.macho.x86_64 -o %t.dwarf.dSYM
62
RUN: dsymutil -accelerator=Apple -oso-prepend-path=%p/.. %p/../Inputs/basic.macho.x86_64 -o %t.apple.dSYM
73

‎llvm/test/tools/dsymutil/X86/update-one-CU.test

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
UNSUPPORTED: system-windows
2-
Windows does not like renaming files that have open handles to them. We
3-
need to use TempFile::keep to move them in a portable way.
4-
51
RUN: dsymutil -oso-prepend-path=%p/.. %p/../Inputs/objc.macho.x86_64 -o %t.dSYM
62
RUN: dsymutil -update %t.dSYM
73
RUN: llvm-dwarfdump -apple-types -apple-objc %t.dSYM | FileCheck %s

‎llvm/test/tools/dsymutil/X86/update.test

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
UNSUPPORTED: system-windows
2-
Windows does not like renaming files that have open handles to them. We
3-
need to use TempFile::keep to move them in a portable way.
4-
51
RUN: rm -rf %t.dir
62
RUN: mkdir -p %t.dir
73
RUN: cat %p/../Inputs/basic.macho.x86_64 > %t.dir/basic

‎llvm/tools/dsymutil/MachOUtils.cpp

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,27 @@ namespace llvm {
2727
namespace dsymutil {
2828
namespace MachOUtils {
2929

30+
llvm::Error ArchAndFile::createTempFile() {
31+
llvm::SmallString<128> TmpModel;
32+
llvm::sys::path::system_temp_directory(true, TmpModel);
33+
llvm::sys::path::append(TmpModel, "dsym.tmp%%%%%.dwarf");
34+
Expected<sys::fs::TempFile> T = sys::fs::TempFile::create(TmpModel);
35+
36+
if (!T)
37+
return T.takeError();
38+
39+
File = llvm::Optional<sys::fs::TempFile>(std::move(*T));
40+
return Error::success();
41+
}
42+
43+
llvm::StringRef ArchAndFile::path() const { return File->TmpName; }
44+
45+
ArchAndFile::~ArchAndFile() {
46+
if (File)
47+
if (auto E = File->discard())
48+
llvm::consumeError(std::move(E));
49+
}
50+
3051
std::string getArchName(StringRef Arch) {
3152
if (Arch.startswith("thumb"))
3253
return (llvm::Twine("arm") + Arch.drop_front(5)).str();
@@ -53,21 +74,16 @@ static bool runLipo(StringRef SDKPath, SmallVectorImpl<StringRef> &Args) {
5374
return true;
5475
}
5576

56-
bool generateUniversalBinary(SmallVectorImpl<ArchAndFilename> &ArchFiles,
77+
bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
5778
StringRef OutputFileName,
5879
const LinkOptions &Options, StringRef SDKPath) {
59-
// No need to merge one file into a universal fat binary. First, try
60-
// to move it (rename) to the final location. If that fails because
61-
// of cross-device link issues then copy and delete.
80+
// No need to merge one file into a universal fat binary.
6281
if (ArchFiles.size() == 1) {
63-
StringRef From(ArchFiles.front().Path);
64-
if (sys::fs::rename(From, OutputFileName)) {
65-
if (std::error_code EC = sys::fs::copy_file(From, OutputFileName)) {
66-
WithColor::error() << "while copying " << From << " to "
67-
<< OutputFileName << ": " << EC.message() << "\n";
68-
return false;
69-
}
70-
sys::fs::remove(From);
82+
if (auto E = ArchFiles.front().File->keep(OutputFileName)) {
83+
WithColor::error() << "while keeping " << ArchFiles.front().path()
84+
<< " as " << OutputFileName << ": "
85+
<< toString(std::move(E)) << "\n";
86+
return false;
7187
}
7288
return true;
7389
}
@@ -77,7 +93,7 @@ bool generateUniversalBinary(SmallVectorImpl<ArchAndFilename> &ArchFiles,
7793
Args.push_back("-create");
7894

7995
for (auto &Thin : ArchFiles)
80-
Args.push_back(Thin.Path);
96+
Args.push_back(Thin.path());
8197

8298
// Align segments to match dsymutil-classic alignment
8399
for (auto &Thin : ArchFiles) {

‎llvm/tools/dsymutil/MachOUtils.h

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#define LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H
1111

1212
#include "llvm/ADT/StringRef.h"
13+
#include "llvm/Support/FileSystem.h"
1314
#include <string>
1415

1516
namespace llvm {
@@ -20,12 +21,20 @@ class DebugMap;
2021
struct LinkOptions;
2122
namespace MachOUtils {
2223

23-
struct ArchAndFilename {
24-
std::string Arch, Path;
25-
ArchAndFilename(StringRef Arch, StringRef Path) : Arch(Arch), Path(Path) {}
24+
struct ArchAndFile {
25+
std::string Arch;
26+
// Optional because TempFile has no default constructor.
27+
Optional<llvm::sys::fs::TempFile> File;
28+
29+
llvm::Error createTempFile();
30+
llvm::StringRef path() const;
31+
32+
ArchAndFile(StringRef Arch) : Arch(Arch) {}
33+
ArchAndFile(ArchAndFile &&A) = default;
34+
~ArchAndFile();
2635
};
2736

28-
bool generateUniversalBinary(SmallVectorImpl<ArchAndFilename> &ArchFiles,
37+
bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
2938
StringRef OutputFileName, const LinkOptions &,
3039
StringRef SDKPath);
3140

‎llvm/tools/dsymutil/dsymutil.cpp

Lines changed: 11 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -313,13 +313,6 @@ static std::string getOutputFileName(llvm::StringRef InputFile) {
313313
return BundleDir.str();
314314
}
315315

316-
static Expected<sys::fs::TempFile> createTempFile() {
317-
llvm::SmallString<128> TmpModel;
318-
llvm::sys::path::system_temp_directory(true, TmpModel);
319-
llvm::sys::path::append(TmpModel, "dsym.tmp%%%%%.dwarf");
320-
return sys::fs::TempFile::create(TmpModel);
321-
}
322-
323316
/// Parses the command line options into the LinkOptions struct and performs
324317
/// some sanity checking. Returns an error in case the latter fails.
325318
static Expected<LinkOptions> getOptions() {
@@ -400,18 +393,6 @@ static Expected<std::vector<std::string>> getInputs(bool DsymAsInput) {
400393
return Inputs;
401394
}
402395

403-
namespace {
404-
struct TempFileVector {
405-
std::vector<sys::fs::TempFile> Files;
406-
~TempFileVector() {
407-
for (sys::fs::TempFile &Tmp : Files) {
408-
if (Error E = Tmp.discard())
409-
errs() << toString(std::move(E));
410-
}
411-
}
412-
};
413-
} // namespace
414-
415396
int main(int argc, char **argv) {
416397
InitLLVM X(argc, argv);
417398

@@ -523,8 +504,7 @@ int main(int argc, char **argv) {
523504
!DumpDebugMap && (OutputFileOpt != "-") &&
524505
(DebugMapPtrsOrErr->size() != 1 || OptionsOrErr->Update);
525506

526-
llvm::SmallVector<MachOUtils::ArchAndFilename, 4> TempFiles;
527-
TempFileVector TempFileStore;
507+
llvm::SmallVector<MachOUtils::ArchAndFile, 4> TempFiles;
528508
std::atomic_char AllOK(1);
529509
for (auto &Map : *DebugMapPtrsOrErr) {
530510
if (Verbose || DumpDebugMap)
@@ -543,16 +523,18 @@ int main(int argc, char **argv) {
543523
std::shared_ptr<raw_fd_ostream> OS;
544524
std::string OutputFile = getOutputFileName(InputFile);
545525
if (NeedsTempFiles) {
546-
Expected<sys::fs::TempFile> T = createTempFile();
547-
if (!T) {
548-
errs() << toString(T.takeError());
526+
TempFiles.emplace_back(Map->getTriple().getArchName().str());
527+
528+
auto E = TempFiles.back().createTempFile();
529+
if (E) {
530+
errs() << toString(std::move(E));
549531
return 1;
550532
}
551-
OS = std::make_shared<raw_fd_ostream>(T->FD, /*shouldClose*/ false);
552-
OutputFile = T->TmpName;
553-
TempFileStore.Files.push_back(std::move(*T));
554-
TempFiles.emplace_back(Map->getTriple().getArchName().str(),
555-
OutputFile);
533+
534+
auto &TempFile = *(TempFiles.back().File);
535+
OS = std::make_shared<raw_fd_ostream>(TempFile.FD,
536+
/*shouldClose*/ false);
537+
OutputFile = TempFile.TmpName;
556538
} else {
557539
std::error_code EC;
558540
OS = std::make_shared<raw_fd_ostream>(NoOutput ? "-" : OutputFile, EC,

0 commit comments

Comments
 (0)
Please sign in to comment.