[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

437

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

437

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

437

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

437

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

437

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

613

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> getMemoryBuffer() {
     return check(MemoryBuffer::getFile(Path, /*FileSize=*/-1,
                                        /*RequiresNullTerminator=*/false,
                                        /*IsVolatileSize=*/true),
                  "could not open " + Path);
   }
 
   std::string Path;
 };
+
+class TemporaryFileMemoryBuffer : public MemoryBuffer {
+  TemporaryFile File;
+  std::unique_ptr<MemoryBuffer> Buffer;
+public:
+  TemporaryFileMemoryBuffer(TemporaryFile&& file)
+    : MemoryBuffer() , File(std::move(file)) , Buffer(File.getMemoryBuffer()) {
+    init(Buffer->getBufferStart(), Buffer->getBufferEnd(), false);
+  }
+  /// Return an identifier for this buffer, typically the filename it was read
+  /// from.
+  virtual StringRef getBufferIdentifier() const override {
+    return Buffer->getBufferIdentifier();
+  }
+
+  /// Return information on the memory mechanism used to support the
+  /// MemoryBuffer.
+  virtual BufferKind getBufferKind() const override {
+    return Buffer->getBufferKind();
+  }
+};
 }
 
 // Create the default manifest file as a temporary file.
 TemporaryFile createDefaultXml() {
   // Create a temporary file.
   TemporaryFile File("defaultxml", "manifest");
 
   // Open the temporary file for writing.
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();
+  return llvm::make_unique<TemporaryFileMemoryBuffer>(std::move(ResFile));
 }
 
 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) {
   // 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());
   E.run();
   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"