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

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

Can we assert success here?

589

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

Done.

300

Done.

589

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

[rnk]

COFF/DriverUtils.cpp

589

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

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

300

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

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

300

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

[rnk]

COFF/DriverUtils.cpp

307

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

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

307

s/no Windows/on Windows/

309

s/enforces/forces/

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:
+  explicit 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;
+    sys::fs::remove(Path);
+    assert(!sys::fs::exists(Path));
+  }
+
+  std::unique_ptr<MemoryBuffer> getMemBufferCopy() {
+    std::unique_ptr<MemoryBuffer> MB =
+        check(MemoryBuffer::getFile(Path), "could not open " + Path);
+    return MemoryBuffer::getMemBufferCopy(MB->getBuffer(), 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.getMemBufferCopy();
 }
 
 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.getMemBufferCopy();
 }
 
 // Create OptTable
 
 // Create prefix string literals used in Options.td
 #define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;
 #include "Options.inc"
 #undef PREFIX