Remove temoprary files.

[ruiu]

Summary

Previously, we created temporary files using llvm::sys::fs::createTemporaryFile
and remove them using llvm::FileRemover. This is error-prone as it is easy to
forget creating FileRemover instances after creating temporary files.
There is actually a temporary file leak bug.

This patch introduces a new class, TemporaryFile, to manage temporary files
in the RAII style.

[ruiu]

• Fixed undefined behavior.

[rnk]

I don't think this works as in Windows

COFF/DriverUtils.cpp

286 ↗

(On Diff #70117)

Let's thread through the other parameter, so that we get "resource-NNN.obj". That was somewhat helpful for my debugging purposes, and it will show up in the debug info too.

300 ↗

(On Diff #70117)

Can we assert success here?

584 ↗

(On Diff #70117)

Won't this fail on Windows, where you cannot remove a file which has an open mapping?

[ruiu]

• Updated as per rnk's comments.

COFF/DriverUtils.cpp

286 ↗

(On Diff #70117)

Done.

300 ↗

(On Diff #70117)

Done.

584 ↗

(On Diff #70117)

Surprisingly enough, it can actually deletes files. I double-checked that. Don't have a good explanation yet though.

[rnk]

COFF/DriverUtils.cpp

585 ↗

(On Diff #70128)

I think the explanation is that MemoryBuffer only uses MapViewOfFile when shouldUseMmap returns true (if the file is small or its size is a multiple of 4K and RequiresNullTerminator is set). I suspect if you arrange for it to return true, then this removal will fail.

[ruiu]

• Add TemporaryFile::getMemBufferCopy to create a copy of a memory buffer.

[amccarth]

I know that in theory, TemporaryFile is very simple and unlikely to fail. In practice, though, Windows can be very finicky about deleting files, so it might be nice to add a test for TemporaryFile. I'm surprised LLVM doesn't already have such a class that's already well-tested.

COFF/DriverUtils.cpp

286 ↗

(On Diff #70128)

I don't think the explicit does anything here, since the constructor requires two parameters.

300 ↗

(On Diff #70128)

I'm concerned the assertion could fail sporadically on Windows, since there's a delay between asking for the file to be deleted and the actual deletion of the file.

[ruiu]

• Address Adrian's comments.

[ruiu]

I think TemporaryFile class is well exercised by existing lit tests, but I can add a unit test if you want.

COFF/DriverUtils.cpp

286 ↗

(On Diff #70178)

I forgot to remove it when I added the second parameter. Removed.

300 ↗

(On Diff #70178)

I think the right thing to do here is to check the return value of remove(), so did I.

[rnk]

COFF/DriverUtils.cpp

307 ↗

(On Diff #70176)

Rather than doing map & copy, you can force MemoryBuffer::getFile to use read/ReadFile the file by passing IsVolatileSize=true. That parameter should probably be named DontUseMmap or ForceReadFile. At the time I think people were trying to document *why* you might want to not use mmap.

You should probably pass the extra parameters anyway, because we definitely don't need RequiresNullTerminator=true for an object file.

[amccarth]

LGTM

[ruiu]

• Address rnk's comments.

[rnk]

lgtm

COFF/DriverUtils.cpp

300 ↗

(On Diff #70181)

If this turns out to not be reliable, we can reconsider the error check later

307 ↗

(On Diff #70181)

s/no Windows/on Windows/

309 ↗

(On Diff #70181)

s/enforces/forces/

lld/trunk/COFF/DriverUtils.cpp

for (int I = 0, E = Line.size(); I != E; ++I) {
       } else {
         Out << Line[I];
       }
     }
     Out << "\"\n";
   }
 }
 
+// An RAII temporary file class that automatically removes a temporary file.
+namespace {
+class TemporaryFile {
+public:
+  TemporaryFile(StringRef Prefix, StringRef Extn) {
+    SmallString<128> S;
+    if (auto EC = sys::fs::createTemporaryFile("lld-" + Prefix, Extn, S))
+      fatal(EC, "cannot create a temporary file");
+    Path = S.str();
+  }
+
+  TemporaryFile(TemporaryFile &&Obj) {
+    std::swap(Path, Obj.Path);
+  }
+
+  ~TemporaryFile() {
+    if (Path.empty())
+      return;
+    if (sys::fs::remove(Path))
+      fatal("failed to remove " + Path);
+  }
+
+  // Returns a memory buffer of this temporary file.
+  // Note that this function does not leave the file open,
+  // so it is safe to remove the file immediately after this function
+  // is called (you cannot remove an opened file on Windows.)
+  std::unique_ptr<MemoryBuffer> getMemoryBuffer() {
+    // IsVolatileSize=true forces MemoryBuffer to not use mmap().
+    return check(MemoryBuffer::getFile(Path, /*FileSize=*/-1,
+                                       /*RequiresNullTerminator=*/false,
+                                       /*IsVolatileSize=*/true),
+                 "could not open " + Path);
+  }
+
+  std::string Path;
+};
+}
+
 // Create the default manifest file as a temporary file.
-static std::string createDefaultXml() {
+TemporaryFile createDefaultXml() {
   // Create a temporary file.
-  SmallString<128> Path;
-  if (auto EC = sys::fs::createTemporaryFile("tmp", "manifest", Path))
-    fatal(EC, "cannot create a temporary file");
+  TemporaryFile File("defaultxml", "manifest");
 
   // Open the temporary file for writing.
   std::error_code EC;
-  llvm::raw_fd_ostream OS(Path, EC, sys::fs::F_Text);
+  llvm::raw_fd_ostream OS(File.Path, EC, sys::fs::F_Text);
   if (EC)
-    fatal(EC, "failed to open " + Path);
+    fatal(EC, "failed to open " + File.Path);
 
   // Emit the XML. Note that we do *not* verify that the XML attributes are
   // syntactically correct. This is intentional for link.exe compatibility.
   OS << "<?xml version=\"1.0\" standalone=\"yes\"?>\n"
      << "<assembly xmlns=\"urn:schemas-microsoft-com:asm.v1\"\n"
      << "          manifestVersion=\"1.0\">\n";
   if (Config->ManifestUAC) {
     OS << "  <trustInfo>\n"
if (!Config->ManifestDependency.empty()) {
          << "    <dependentAssembly>\n"
          << "      <assemblyIdentity " << Config->ManifestDependency << " />\n"
          << "    </dependentAssembly>\n"
          << "  </dependency>\n";
     }
   }
   OS << "</assembly>\n";
   OS.close();
-  return StringRef(Path);
+  return File;
 }
 
 static std::string readFile(StringRef Path) {
   std::unique_ptr<MemoryBuffer> MB =
       check(MemoryBuffer::getFile(Path), "could not open " + Path);
-  std::unique_ptr<MemoryBuffer> Buf(std::move(MB));
-  return Buf->getBuffer();
+  return MB->getBuffer();
 }
 
 static std::string createManifestXml() {
   // Create the default manifest file.
-  std::string Path1 = createDefaultXml();
+  TemporaryFile File1 = createDefaultXml();
   if (Config->ManifestInput.empty())
-    return readFile(Path1);
+    return readFile(File1.Path);
 
   // If manifest files are supplied by the user using /MANIFESTINPUT
   // option, we need to merge them with the default manifest.
-  SmallString<128> Path2;
-  if (auto EC = sys::fs::createTemporaryFile("tmp", "manifest", Path2))
-    fatal(EC, "cannot create a temporary file");
-  FileRemover Remover1(Path1);
-  FileRemover Remover2(Path2);
+  TemporaryFile File2("user", "manifest");
 
   Executor E("mt.exe");
   E.add("/manifest");
-  E.add(Path1);
+  E.add(File1.Path);
   for (StringRef Filename : Config->ManifestInput) {
     E.add("/manifest");
     E.add(Filename);
   }
   E.add("/nologo");
-  E.add("/out:" + StringRef(Path2));
+  E.add("/out:" + StringRef(File2.Path));
   E.run();
-  return readFile(Path2);
+  return readFile(File2.Path);
 }
 
 // Create a resource file containing a manifest XML.
 std::unique_ptr<MemoryBuffer> createManifestRes() {
   // Create a temporary file for the resource script file.
-  SmallString<128> RCPath;
-  if (auto EC = sys::fs::createTemporaryFile("tmp", "rc", RCPath))
-    fatal(EC, "cannot create a temporary file");
-  FileRemover RCRemover(RCPath);
+  TemporaryFile RCFile("manifest", "rc");
 
   // Open the temporary file for writing.
   std::error_code EC;
-  llvm::raw_fd_ostream Out(RCPath, EC, sys::fs::F_Text);
+  llvm::raw_fd_ostream Out(RCFile.Path, EC, sys::fs::F_Text);
   if (EC)
-    fatal(EC, "failed to open " + RCPath);
+    fatal(EC, "failed to open " + RCFile.Path);
 
   // Write resource script to the RC file.
   Out << "#define LANG_ENGLISH 9\n"
       << "#define SUBLANG_DEFAULT 1\n"
       << "#define APP_MANIFEST " << Config->ManifestID << "\n"
       << "#define RT_MANIFEST 24\n"
       << "LANGUAGE LANG_ENGLISH, SUBLANG_DEFAULT\n"
       << "APP_MANIFEST RT_MANIFEST {\n";
   quoteAndPrint(Out, createManifestXml());
   Out << "}\n";
   Out.close();
 
   // Create output resource file.
-  SmallString<128> ResPath;
-  if (auto EC = sys::fs::createTemporaryFile("tmp", "res", ResPath))
-    fatal(EC, "cannot create a temporary file");
+  TemporaryFile ResFile("output-resource", "res");
 
   Executor E("rc.exe");
   E.add("/fo");
-  E.add(ResPath.str());
+  E.add(ResFile.Path);
   E.add("/nologo");
-  E.add(RCPath.str());
+  E.add(RCFile.Path);
   E.run();
-  return check(MemoryBuffer::getFile(ResPath), "could not open " + ResPath);
+  return ResFile.getMemoryBuffer();
 }
 
 void createSideBySideManifest() {
   std::string Path = Config->ManifestFile;
   if (Path == "")
     Path = Config->OutputFile + ".manifest";
   std::error_code EC;
   llvm::raw_fd_ostream Out(Path, EC, llvm::sys::fs::F_Text);
void checkFailIfMismatch(StringRef Arg) {
   Config->MustMatch[K] = V;
 }
 
 // Convert Windows resource files (.res files) to a .obj file
 // using cvtres.exe.
 std::unique_ptr<MemoryBuffer>
 convertResToCOFF(const std::vector<MemoryBufferRef> &MBs) {
   // Create an output file path.
-  SmallString<128> Path;
-  if (auto EC = llvm::sys::fs::createTemporaryFile("resource", "obj", Path))
-    fatal(EC, "could not create temporary file");
+  TemporaryFile File("resource-file", "obj");
 
   // Execute cvtres.exe.
   Executor E("cvtres.exe");
   E.add("/machine:" + machineToStr(Config->Machine));
   E.add("/readonly");
   E.add("/nologo");
-  E.add("/out:" + Path);
+  E.add("/out:" + Twine(File.Path));
   for (MemoryBufferRef MB : MBs)
     E.add(MB.getBufferIdentifier());
   E.run();
-  return check(MemoryBuffer::getFile(Path), "could not open " + Path);
+  return File.getMemoryBuffer();
 }
 
 // Create OptTable
 
 // Create prefix string literals used in Options.td
 #define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;
 #include "Options.inc"
 #undef PREFIX