[dsymutil] Simplify temporary file handling.

[JDevlieghere]

Summary

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.

[JDevlieghere]

llvm/lib/Support/Path.cpp

1163 ↗

(On Diff #157495)

Should we have something similar for Windows?

[jmorse]

LGTM, I get 1 Unsupported (due to Darwin) and 44 successful passes when running lit over the dsymutil\X86\ tests, Windows 10 with a MSVC 2015 build environment. No failures.

Not clear what to do about having MoveFileEx move between volumes, see inline comment, it might best to add the "copy allowed" flag in a follow up commit that's reviewed by someone more familiar with LLVMs filesystem handling.

llvm/lib/Support/Path.cpp

1163 ↗

(On Diff #157495)

The Windows implementation uses MoveFileEx and explicitly supports that kind of rename:

https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-movefileexa

But it wants a flag, "MOVEFILE_COPY_ALLOWED" in the call to MoveFileEx in that case. Adding that to the MoveFileExW call in Windows' Path.inc would be sufficient: off the top of my head I can't imagine circumstances where this interfered with other LLVM code.

[jmorse]

That's 1 Unsupported 43 Passes (apparently I can't count).

[stella.stamenova]

All three tests started failing in our environment after this change:

Failing Tests (3):

LLVM :: tools/dsymutil/X86/accelerator.test
LLVM :: tools/dsymutil/X86/update-one-CU.test
LLVM :: tools/dsymutil/X86/update.test

The error for all three is:

Error : while keeping C:\Windows\SERVIC~2\NETWOR~1\AppData\Local\Temp\lit_tmp_l55xwfv6\dsym.tmp18368.dwarf as E:\_work\14\b\LLVMBuild\test\tools\dsymutil\X86\Output\accelerator.test.tmp.apple.dSYM\Contents\Resources\DWARF\basic.macho.x86_64: The system cannot move the file to a different disk drive.

[stella.stamenova]

By the way, our tests on this particular machine run on the E: drive and it looks like the temporary file is created on the C: drive. This is the command that ends up causing the failure:

"E:\_work\14\b\LLVMBuild\Release\bin\dsymutil.EXE" "--update" "E:\_work\14\b\LLVMBuild\test\tools\dsymutil\X86\Output\update.test.tmp.dir/basic.dSYM"

[zturner]

That sounds to me like a bug in LLVM support libraries. TempFile should
be able to work across drives. It's also a Windows specific bug, so
perhaps the best thing to do is to fix this bug in LLVM.

[jmorse]

Thanks for the report, it looks like just adding the "copy between volumes" flag to windows' rename was insufficient -- a preceding rename attempt (with slightly different semantics) needs to fall through to it.

I've uploaded https://reviews.llvm.org/D50048 which appears to fix this locally for me (Win10), @stella.stamenova if you could confirm whether it fixes it for you?

llvm/trunk/lib/Support/Path.cpp

#ifdef _WIN32
   std::error_code RenameEC = setDeleteDisposition(H, false);
   if (!RenameEC)
     RenameEC = rename_fd(FD, Name);
   // If we can't rename, discard the temporary file.
   if (RenameEC)
     setDeleteDisposition(H, true);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
-  // If we can't rename, discard the temporary file.
+  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
 
   if (!RenameEC)
     TmpName = "";
 
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());

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

for (unsigned Retry = 0; Retry != 200; ++Retry) {
     if (EC ==
         std::error_code(ERROR_CALL_NOT_IMPLEMENTED, std::system_category())) {
       // Wine doesn't support SetFileInformationByHandle in rename_internal.
       // Fall back to MoveFileEx.
       SmallVector<wchar_t, MAX_PATH> WideFrom;
       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());
     }
 
     if (!EC || EC != errc::permission_denied)
       return EC;
 
     // The destination file probably exists and is currently open in another

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

-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
 
 RUN: llvm-dwarfdump -verify %t.dwarf.dSYM
 RUN: llvm-dwarfdump -verify %t.apple.dSYM
 
 RUN: llvm-dwarfdump -debug-names %t.dwarf.dSYM | FileCheck %s -check-prefix=NAMES -check-prefix=DWARF
 RUN: llvm-dwarfdump -apple-names -apple-namespaces -apple-types %t.apple.dSYM | FileCheck %s -check-prefix=NAMES -check-prefix=APPLE

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

-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
 
 CHECK: .apple_types contents:
 CHECK: Hash 0x2b5e6 [
 CHECK-NEXT:    Name@0x145 {
 CHECK-NEXT:      String: 0x00000066 "A"

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

-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
 RUN: dsymutil -oso-prepend-path=%p/.. %t.dir/basic
 RUN: llvm-dwarfdump -a %t.dir/basic.dSYM | FileCheck %S/basic-linking-x86.test
 RUN: dsymutil --update %t.dir/basic.dSYM
 RUN: llvm-dwarfdump -a %t.dir/basic.dSYM | FileCheck %S/basic-linking-x86.test
 RUN: dsymutil -u %t.dir/basic.dSYM
 RUN: llvm-dwarfdump -a %t.dir/basic.dSYM | FileCheck %S/basic-linking-x86.test
 RUN: dsymutil --update %t.dir/basic.dSYM -o %t.dir/updated.dSYM
 RUN: llvm-dwarfdump -a %t.dir/updated.dSYM | FileCheck %S/basic-linking-x86.test
 
 RUN: dsymutil -f -o %t2 -oso-prepend-path=%p/.. %p/../Inputs/basic.macho.x86_64
 RUN: dsymutil -f -u %t2 -o %t3
 RUN: llvm-dwarfdump -a %t3 | FileCheck %S/basic-linking-x86.test

llvm/trunk/tools/dsymutil/MachOUtils.h

 //===-- MachOUtils.h - Mach-o specific helpers for dsymutil  --------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H
 #define LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
 #include <string>
 
 namespace llvm {
 class MCStreamer;
 class raw_fd_ostream;
 namespace dsymutil {
 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);
 
 bool generateDsymCompanion(const DebugMap &DM, MCStreamer &MS,
                            raw_fd_ostream &OutFile);
 
 std::string getArchName(StringRef Arch);
 } // namespace MachOUtils
 } // namespace dsymutil
 } // namespace llvm
 #endif // LLVM_TOOLS_DSYMUTIL_MACHOUTILS_H

llvm/trunk/tools/dsymutil/MachOUtils.cpp

 #include "llvm/Support/Program.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 
 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();
   return Arch;
 }
 
 static bool runLipo(StringRef SDKPath, SmallVectorImpl<StringRef> &Args) {
   auto Path = sys::findProgramByName("lipo", makeArrayRef(SDKPath));
static bool runLipo(StringRef SDKPath, SmallVectorImpl<StringRef> &Args) {
   if (result) {
     WithColor::error() << "lipo: " << ErrMsg << "\n";
     return false;
   }
 
   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 true;
   }
 
   SmallVector<StringRef, 8> Args;
   Args.push_back("lipo");
   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) {
     Thin.Arch = getArchName(Thin.Arch);
     Args.push_back("-segalign");
     Args.push_back(Thin.Arch);
     Args.push_back("20");
   }

llvm/trunk/tools/dsymutil/dsymutil.cpp

static std::string getOutputFileName(llvm::StringRef InputFile) {
   if (!createBundleDir(BundleDir) || !createPlistFile(DwarfFile, BundleDir))
     return "";
 
   llvm::sys::path::append(BundleDir, "Contents", "Resources", "DWARF",
                           llvm::sys::path::filename(DwarfFile));
   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() {
   LinkOptions Options;
 
   Options.Verbose = Verbose;
   Options.NoOutput = NoOutput;
   Options.NoODR = NoODR;
while (DirIt != DirEnd) {
       DirIt.increment(EC);
       if (EC)
         return errorCodeToError(EC);
     }
   }
   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);
 
   void *P = (void *)(intptr_t)getOutputFileName;
   std::string SDKPath = llvm::sys::fs::getMainExecutable(argv[0], P);
   SDKPath = llvm::sys::path::parent_path(SDKPath);
 
   HideUnrelatedOptions({&DsymCategory, &ColorCategory});
for (auto &InputFile : *InputsOrErr) {
     llvm::ThreadPool Threads(NumThreads);
 
     // If there is more than one link to execute, we need to generate
     // temporary files.
     bool NeedsTempFiles =
         !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)
         Map->print(llvm::outs());
 
       if (DumpDebugMap)
         continue;
 
       if (Map->begin() == Map->end())
         WithColor::warning()
             << "no debug symbols in executable (-arch "
             << MachOUtils::getArchName(Map->getTriple().getArchName()) << ")\n";
 
       // Using a std::shared_ptr rather than std::unique_ptr because move-only
       // types don't work with std::bind in the ThreadPool implementation.
       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,
                                               sys::fs::F_None);
         if (EC) {
           errs() << OutputFile << ": " << EC.message();
           return 1;
         }