[llvm] a04879c - [dsymutil] Use createTemporaryFile instead of TempFile

[Jonas Devlieghere via llvm-commits]

Author: Jonas Devlieghere
Date: 2023-08-17T11:37:37-07:00
New Revision: a04879ce7dd6410fca7df25c882406eacf8397f5

URL: https://github.com/llvm/llvm-project/commit/a04879ce7dd6410fca7df25c882406eacf8397f5
DIFF: https://github.com/llvm/llvm-project/commit/a04879ce7dd6410fca7df25c882406eacf8397f5.diff

LOG: [dsymutil] Use createTemporaryFile instead of TempFile

Use createTemporaryFile in favor of the TempFile abstraction to ensure
we have an on-disk file. This fixes an issue on Windows where the DWARF
verifier would fail when trying to open the temporary file from disk.

Added: 
    

Modified: 
    llvm/tools/dsymutil/MachOUtils.cpp
    llvm/tools/dsymutil/MachOUtils.h
    llvm/tools/dsymutil/dsymutil.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/tools/dsymutil/MachOUtils.cpp b/llvm/tools/dsymutil/MachOUtils.cpp
index 953c7e960f0832..44c925fce5c2b5 100644
--- a/llvm/tools/dsymutil/MachOUtils.cpp
+++ b/llvm/tools/dsymutil/MachOUtils.cpp
@@ -10,6 +10,7 @@
 #include "BinaryHolder.h"
 #include "DebugMap.h"
 #include "LinkUtils.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/CodeGen/NonRelocatableStringpool.h"
 #include "llvm/MC/MCAsmLayout.h"
 #include "llvm/MC/MCAssembler.h"
@@ -29,24 +30,30 @@ 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);
+  SmallString<256> SS;
+  std::error_code EC = sys::fs::createTemporaryFile("dsym", "dwarf", FD, SS);
 
-  if (!T)
-    return T.takeError();
+  if (EC)
+    return errorCodeToError(EC);
+
+  Path = SS.str();
 
-  File = std::make_unique<sys::fs::TempFile>(std::move(*T));
   return Error::success();
 }
 
-llvm::StringRef ArchAndFile::path() const { return File->TmpName; }
+llvm::StringRef ArchAndFile::getPath() const {
+  assert(!Path.empty() && "path called before createTempFile");
+  return Path;
+}
+
+int ArchAndFile::getFD() const {
+  assert((FD != -1) && "path called before createTempFile");
+  return FD;
+}
 
 ArchAndFile::~ArchAndFile() {
-  if (File)
-    if (auto E = File->discard())
-      llvm::consumeError(std::move(E));
+  if (!Path.empty())
+    sys::fs::remove(Path);
 }
 
 std::string getArchName(StringRef Arch) {
@@ -82,11 +89,17 @@ bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
                              bool Fat64) {
   // No need to merge one file into a universal fat binary.
   if (ArchFiles.size() == 1) {
-    if (auto E = ArchFiles.front().File->keep(OutputFileName)) {
-      WithColor::error() << "while keeping " << ArchFiles.front().path()
-                         << " as " << OutputFileName << ": "
-                         << toString(std::move(E)) << "\n";
-      return false;
+    llvm::StringRef TmpPath = ArchFiles.front().getPath();
+    if (auto EC = sys::fs::rename(TmpPath, OutputFileName)) {
+      // If we can't rename, try to copy to work around cross-device link
+      // issues.
+      EC = sys::fs::copy_file(TmpPath, OutputFileName);
+      if (EC) {
+        WithColor::error() << "while keeping " << TmpPath << " as "
+                           << OutputFileName << ": " << EC.message() << "\n";
+        return false;
+      }
+      sys::fs::remove(TmpPath);
     }
     return true;
   }
@@ -96,7 +109,7 @@ bool generateUniversalBinary(SmallVectorImpl<ArchAndFile> &ArchFiles,
   Args.push_back("-create");
 
   for (auto &Thin : ArchFiles)
-    Args.push_back(Thin.path());
+    Args.push_back(Thin.getPath());
 
   // 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 9d3581348853ae..059d9fdd788a66 100644
--- a/llvm/tools/dsymutil/MachOUtils.h
+++ b/llvm/tools/dsymutil/MachOUtils.h
@@ -26,10 +26,12 @@ namespace MachOUtils {
 
 struct ArchAndFile {
   std::string Arch;
-  std::unique_ptr<llvm::sys::fs::TempFile> File;
+  std::string Path;
+  int FD = -1;
 
   llvm::Error createTempFile();
-  llvm::StringRef path() const;
+  llvm::StringRef getPath() const;
+  int getFD() const;
 
   ArchAndFile(StringRef Arch) : Arch(std::string(Arch)) {}
   ArchAndFile(ArchAndFile &&A) = default;

diff  --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index 20d11bceb05eed..1dffe0edc4dff9 100644
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/llvm/tools/dsymutil/dsymutil.cpp
@@ -772,10 +772,10 @@ int main(int argc, char **argv) {
             return;
           }
 
-          auto &TempFile = *(TempFiles.back().File);
-          OS = std::make_shared<raw_fd_ostream>(TempFile.FD,
+          MachOUtils::ArchAndFile &AF = TempFiles.back();
+          OS = std::make_shared<raw_fd_ostream>(AF.getFD(),
                                                 /*shouldClose*/ false);
-          OutputFile = TempFile.TmpName;
+          OutputFile = AF.getPath();
         } else {
           std::error_code EC;
           OS = std::make_shared<raw_fd_ostream>(
@@ -843,7 +843,8 @@ int main(int argc, char **argv) {
         uint64_t FileOffset =
             MagicAndCountSize + UniversalArchInfoSize * TempFiles.size();
         for (const auto &File : TempFiles) {
-          ErrorOr<vfs::Status> stat = Options.LinkOpts.VFS->status(File.path());
+          ErrorOr<vfs::Status> stat =
+              Options.LinkOpts.VFS->status(File.getPath());
           if (!stat)
             break;
           if (FileOffset > UINT32_MAX) {