[PATCH] D51095: [LLD] [RFC] Don't close the memory mapping when exiting

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo created this revision.
mstorsjo added reviewers: ruiu, pcc, rnk.
Herald added a subscriber: hiraditya.

If exiting due to an error in a multithreaded section of the code, there may be other threads still running, accessing the memory mapped area. Just remove the temporary file (on unix, just remove it, while it has got a remove-on-exit flag set on windows) and close its file descriptor, while keeping the memory mapping intact.

This implements @pcc's suggestion on fixing PR38647.

I'm a little weary about the fact that the existing comment in FileOutputBuffer.cpp says "Close the mapping before deleting the temp file, so that the removal succeeds.", while this approach taken by this patch seems to work fine (at least on windows 10). OTOH we don't actually delete the file on windows, we just close the FD.


Repository:
  rL LLVM

https://reviews.llvm.org/D51095

Files:
  lld/Common/ErrorHandler.cpp
  llvm/include/llvm/Support/FileOutputBuffer.h
  llvm/lib/Support/FileOutputBuffer.cpp


Index: llvm/lib/Support/FileOutputBuffer.cpp
===================================================================
--- llvm/lib/Support/FileOutputBuffer.cpp
+++ llvm/lib/Support/FileOutputBuffer.cpp
@@ -61,6 +61,13 @@
     consumeError(Temp.discard());
   }
 
+  void discard() override {
+    // Delete the temp file if it still was open, but keeping the mapping
+    // active. (Closing the FD while keeping the mapping alive seems to work
+    // on windows as well.)
+    consumeError(Temp.discard());
+  }
+
 private:
   std::unique_ptr<fs::mapped_file_region> Buffer;
   fs::TempFile Temp;
Index: llvm/include/llvm/Support/FileOutputBuffer.h
===================================================================
--- llvm/include/llvm/Support/FileOutputBuffer.h
+++ llvm/include/llvm/Support/FileOutputBuffer.h
@@ -76,6 +76,10 @@
   /// deallocates the buffer and the target file is never written.
   virtual ~FileOutputBuffer() {}
 
+  /// This removes the temporary file (unless it already was committed)
+  /// but keeps the memory mapping alive.
+  virtual void discard() {}
+
 protected:
   FileOutputBuffer(StringRef Path) : FinalPath(Path) {}
 
Index: lld/Common/ErrorHandler.cpp
===================================================================
--- lld/Common/ErrorHandler.cpp
+++ lld/Common/ErrorHandler.cpp
@@ -48,7 +48,8 @@
 
 void lld::exitLld(int Val) {
   // Delete the output buffer so that any tempory file is deleted.
-  errorHandler().OutputBuffer.reset();
+  if (errorHandler().OutputBuffer)
+    errorHandler().OutputBuffer->discard();
 
   // Dealloc/destroy ManagedStatic variables before calling
   // _exit(). In a non-LTO build, this is a nop. In an LTO


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51095.161916.patch
Type: text/x-patch
Size: 1682 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180822/c2caeab1/attachment.bin>