[COFF] Fix manifest resource file creation on Windows

[Ilod]

Summary

createManifestRes was generating a MemoryBuffer from a TemporaryFile, keeping the data but removing the file, before passing the file path to CVTRES.exe, leading to the following error:
CVTRES : fatal error CVT1101: cannot open 'C:\Users\user\AppData\Local\Temp\lld-output-resource-bfee19.res' for reading
With this, we instead create a new TemporaryFile before passing it to cvtres.

[ruiu]

COFF/DriverUtils.cpp

416

Does this work?

return MemoryBuffer::getMemBufferCopy(ResFile.getMemoryBuffer().getBuffer());

This creates a copy of memory buffer that do not refer the original memory buffer, so you don't need to define the subclass of MemoryBuffer.

[Ilod]

COFF/DriverUtils.cpp

416

I don't think so. The point of the subclass is to keep the TemporaryFile alive. In fact in this particular case we don't really care about the MemoryBuffer, we just stick with it because LinkerDriver::link keep a list of MemoryBuffer to reference resources, then call ConvertResToCOFF on this resource list, but only forward the underlying file path to CVTRES (by adding MB.getBufferIdentifier() to its parameters).
Returning a copy of the membuffer won't change that the TemporaryFile objet is destroyed, removing the underlying file, thus making cvtres fail. Having a subclass allow us to move the TemporaryFile in a member, thus not deleting the file before the MemoryBuffer is also destroyed.

[ruiu]

COFF/DriverUtils.cpp

416

I see. Thank you for the explanation. Honestly, I prefer simple code than the runtime efficiency here. How about writing MemoryBuffers to temporary files in convertResToCOFF before passing them to cvtres?

[Ilod]

COFF/DriverUtils.cpp

416

It would also work, and be way simpler, yeah.
It would however probably be a waste on projets with a bunch of resources, as it seems the manifest is the only one which have this problem (because it's the only one we create internally in a TemporaryFile, others are created externally), so we would copy many files already existing.
I don't know if it's really a problem, I never used a project with more than a few resources.

I can make a patch tomorrow if you think it's a better way to do it.

[ruiu]

COFF/DriverUtils.cpp

416

It would be slower for sure, but it is probably negligible. We can optimize later if it turns out to be too slow.

[Ilod]

Not much simpler in fact, because we need to handle a vector of files, but more local, so probably easier to understand.

[ruiu]

LGTM. Thank you for fixing this!

COFF/DriverUtils.cpp

591

Please add a comment why we are doing this; we cannot pass paths of the files because the file may be a temporary file and in that case the path have already been removed.

[Ilod]

Added comment.
I don't have commit rights, so I will depend on you for this.

[ruiu]

Submitted as r287034.

COFF/DriverUtils.cpp

std::unique_ptr<MemoryBuffer> createManifestRes() {
   TemporaryFile ResFile("output-resource", "res");
 
   Executor E("rc.exe");
   E.add("/fo");
   E.add(ResFile.Path);
   E.add("/nologo");
   E.add(RCFile.Path);
   E.run();
   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);
convertResToCOFF(const std::vector<MemoryBufferRef> &MBs) {
   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:" + Twine(File.Path));
-  for (MemoryBufferRef MB : MBs)
-    E.add(MB.getBufferIdentifier());
+
+  // We must create new files because the memory buffers we have may have no
+  // underlying file still existing on the disk.
+  // It happens if it was created from a TemporaryFile, which usually delete
+  // the file just after creating the MemoryBuffer.
+  std::vector<TemporaryFile> ResFiles;
+  ResFiles.reserve(MBs.size());
+  for (MemoryBufferRef MB : MBs) {
+    // We store the temporary file in a vector to avoid deletion
+    // before running cvtres
+    ResFiles.emplace_back("resource-file", "res");
+    TemporaryFile& ResFile = ResFiles.back();
+    // Write the content of the resource in a temporary file
+    std::error_code EC;
+    llvm::raw_fd_ostream OS(ResFile.Path, EC, sys::fs::F_None);
+    if (EC)
+      fatal(EC, "failed to open " + ResFile.Path);
+    OS << MB.getBuffer();
+    OS.close();
+
+    E.add(ResFile.Path);
+  }
+
   E.run();
   return File.getMemoryBuffer();
 }
 
 // Create OptTable
 
 // Create prefix string literals used in Options.td
 #define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;