[PATCH] D26656: [COFF] Fix manifest resource file creation on Windows

[Rui Ueyama via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/DriverUtils.cpp:437
   E.run();
-  return ResFile.getMemoryBuffer();
+  return llvm::make_unique<TemporaryFileMemoryBuffer>(std::move(ResFile));
 }
----------------
Ilod wrote:
> ruiu wrote:
> > 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.
> 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.
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?


https://reviews.llvm.org/D26656