clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded

[thakis]

Summary

Second attempt after http://llvm.org/viewvc/llvm-project?rev=296166&view=rev

In the first attempt, Code (the memory buffer backing the input file) was reset before overwriteChangedFiles() was called, but overwriteChangedFiles() still reads from it.
This time, load the whole input file into memory instead of using mmap when formatting in-place.

(Since the test is identical to what was in the repo before chapuni's revert, svn diff doesn't show it – see the above link for the test.)

[thakis]

(related, and why i picked rnk as reviewer: https://bugs.llvm.org/show_bug.cgi?id=17216 https://bugs.llvm.org/show_bug.cgi?id=17960)

[klimek]

include/clang/Rewrite/Core/Rewriter.h

188–197

llvm.org seems to be down, so I cannot access the original patch - but I'm somewhat opposed to making this interface more complex. If we have a coupling of MemoryBuffer to the Rewriter, can we make that explicit instead?

[thakis]

include/clang/Rewrite/Core/Rewriter.h

188–197

I think this generally requires per-app knowledge on when and how it's safe to free up memory buffers (for example, the fixit rewriter can't easily drop its inputs currently as far as I can tell without further refactorings). If you have a good suggestion, I'm all ears, but this seems like a fairly minor change that attempts to fix a fairly major bug. So if there's some better way of factoring this that I currently don't see, it's almost no work to move to that then.

[klimek]

Can we open the files with FILE_SHARE_DELETE instead?

[klimek]

Also, do we want to not call ReplaceFile in Path.inc on Win if it potentially leaves temp files lying around?
Reading the code, it looks like we currently actually believe it may fail, and retry with MoveFileEx afterwards...

[rnk]

My understanding is that Windows doesn't allow you to delete a file that has been opened and mapped into any process, even if it has been opened with FILE_SHARE_DELETE. One way to work around this would be to avoid memory mapping files in the SourceManager when using the Rewriter. Then FILE_SHARE_DELETE will do what you want. There should be a parameter to MemoryBuffer::getFile to control this. However, I think it really would be cleaner if we unmapped all of our MemoryBuffers of inputs before overwriting them in place. Keeping those buffers around feels like having dangling references.

[amccarth]

lib/Rewrite/Rewriter.cpp

455

I'm not familiar with the structure of this code, but it seems odd to me that the callback is called prewrite when it happens _after_ the write.

[thakis]

Here's a simpler fix, suggested by rnk.

[thakis]

lib/Rewrite/Rewriter.cpp

455

obsolete now, but while it's true that it's called after the write to the temp file, it's called before the temp file is moved to the final location -- and for the client code of this function, the temp file is an implementation detail it doesn't know about, so it's really called before "the" write.

[rnk]

lgtm

We should file a bug against the Rewriter interface. It should really take MemoryBuffers from the caller and handle this for them.

[thakis]

r296408, thanks!

include/clang/Rewrite/Core/Rewriter.h

public:
   const_buffer_iterator buffer_begin() const { return RewriteBuffers.begin(); }
   const_buffer_iterator buffer_end() const { return RewriteBuffers.end(); }
 
   /// overwriteChangedFiles - Save all changed files to disk.
   ///
   /// Returns true if any files were not saved successfully.
   /// Outputs diagnostics via the source manager's diagnostic engine
   /// in case of an error.
-  bool overwriteChangedFiles();
+  ///
+  /// prewrite(FID) is called after all changes to FID have been written to a
+  /// temporary file, but before that temporary file is moved into FID's
+  /// location.  Windows's ReplaceFileW(to, from) internally creates (another)
+  /// temporary file and swaps the data streams of that file with the one of
+  /// |to| so that it's possible to replace an opened file.  However, if |to|
+  /// _was_ opened, the temp file now has that open data stream, and
+  /// ReplaceFileW() fails to delete it, causing a tempo file leak.  To fix,
+  /// clients of this function must close all file mappings of a file before
+  /// it's written to (but after it's been read by Rewriter), and this prewrite
+  /// callback is a convenient place for doing this.
+  bool overwriteChangedFiles(llvm::function_ref<void(FileID)> prewrite);
 
 private:
   unsigned getLocationOffsetAndFileID(SourceLocation Loc, FileID &FID) const;
 };
 
 } // end namespace clang
 
 #endif

lib/Frontend/Rewrite/FixItRewriter.cpp

if (NumFailures > 0 && !FixItOpts->FixWhatYouCan) {
     Diag(FullSourceLoc(), diag::warn_fixit_no_changes);
     return true;
   }
 
   RewritesReceiver Rec(Rewrite);
   Editor.applyRewrites(Rec);
 
   if (FixItOpts->InPlace) {
-    // Overwriting open files on Windows is tricky, but the rewriter can do it
-    // for us.
-    Rewrite.overwriteChangedFiles();
+    // FIXME: This likely leaks temp files on Windows, see comment on
+    // Rewriter::overwriteChangedFiles().
+    Rewrite.overwriteChangedFiles([](FileID){});
     return false;
   }
 
   for (iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
     const FileEntry *Entry = Rewrite.getSourceMgr().getFileEntryForID(I->first);
     int fd;
     std::string Filename = FixItOpts->RewriteFilename(Entry->getName(), fd);
     std::error_code EC;

lib/Rewrite/Rewriter.cpp

private:
   DiagnosticsEngine &Diagnostics;
   StringRef Filename;
   SmallString<128> TempFilename;
   std::unique_ptr<llvm::raw_fd_ostream> FileStream;
   bool &AllWritten;
 };
 } // end anonymous namespace
 
-bool Rewriter::overwriteChangedFiles() {
+bool Rewriter::overwriteChangedFiles(llvm::function_ref<void(FileID)> prewrite) {
   bool AllWritten = true;
   for (buffer_iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
     const FileEntry *Entry =
         getSourceMgr().getFileEntryForID(I->first);
     AtomicallyMovedFile File(getSourceMgr().getDiagnostics(), Entry->getName(),
                              AllWritten);
     if (File.ok()) {
       I->second.write(File.getStream());
+      prewrite(I->first);
     }
   }
   return !AllWritten;
 }

lib/Tooling/Refactoring.cpp

bool RefactoringTool::applyAllReplacements(Rewriter &Rewrite) {
   bool Result = true;
   for (const auto &Entry : groupReplacementsByFile(
            Rewrite.getSourceMgr().getFileManager(), FileToReplaces))
     Result = tooling::applyAllReplacements(Entry.second, Rewrite) && Result;
   return Result;
 }
 
 int RefactoringTool::saveRewrittenFiles(Rewriter &Rewrite) {
-  return Rewrite.overwriteChangedFiles() ? 1 : 0;
+  // FIXME: This likely leaks temp files on Windows, see comment on
+  // Rewriter::overwriteChangedFiles().
+  return Rewrite.overwriteChangedFiles([](FileID){}) ? 1 : 0;
 }
 
 bool formatAndApplyAllReplacements(
     const std::map<std::string, Replacements> &FileToReplaces,
     Rewriter &Rewrite, StringRef Style) {
   SourceManager &SM = Rewrite.getSourceMgr();
   FileManager &Files = SM.getFileManager();
 

tools/clang-format/ClangFormat.cpp

if (OutputXML) {
     SourceManager Sources(Diagnostics, Files);
     FileID ID = createInMemoryFile(AssumedFileName, Code.get(), Sources, Files,
                                    InMemoryFileSystem.get());
     Rewriter Rewrite(Sources, LangOptions());
     tooling::applyAllReplacements(Replaces, Rewrite);
     if (Inplace) {
       if (FileName == "-")
         errs() << "error: cannot use -i when reading from stdin.\n";
-      else if (Rewrite.overwriteChangedFiles())
+      else if (Rewrite.overwriteChangedFiles([&Code, ID](FileID FID) {
+                 assert(FID == ID);
+                 Code.reset();
+               }))
         return true;
     } else {
       if (Cursor.getNumOccurrences() != 0)
         outs() << "{ \"Cursor\": "
                << FormatChanges.getShiftedCodePosition(CursorPosition)
                << ", \"IncompleteFormat\": "
                << (IncompleteFormat ? "true" : "false") << " }\n";
       Rewrite.getEditBuffer(ID).write(outs());

unittests/Tooling/RefactoringTest.cpp

public:
   RewriterTestContext Context;
 };
 
 TEST_F(FlushRewrittenFilesTest, StoresChangesOnDisk) {
   FileID ID = createFile("input.cpp", "line1\nline2\nline3\nline4");
   Replacements Replaces = toReplacements({Replacement(
       Context.Sources, Context.getLocation(ID, 2, 1), 5, "replaced")});
   EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
-  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+  // This test doesn't create file mappings, so overwriteChangedFiles() needs
+  // no parameter.
+  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
   EXPECT_EQ("line1\nreplaced\nline3\nline4",
             getFileContentFromDisk("input.cpp"));
 }
 
 namespace {
 template <typename T>
 class TestVisitor : public clang::RecursiveASTVisitor<T> {
 public:

unittests/Tooling/RewriterTest.cpp

 namespace clang {
 namespace tooling {
 namespace {
 
 TEST(Rewriter, OverwritesChangedFiles) {
   RewriterTestContext Context;
   FileID ID = Context.createOnDiskFile("t.cpp", "line1\nline2\nline3\nline4");
   Context.Rewrite.ReplaceText(Context.getLocation(ID, 2, 1), 5, "replaced");
-  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+  // This test doesn't create file mappings, so overwriteChangedFiles() needs
+  // no parameter.
+  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
   EXPECT_EQ("line1\nreplaced\nline3\nline4",
             Context.getFileContentFromDisk("t.cpp")); 
 }
 
 TEST(Rewriter, ContinuesOverwritingFilesOnError) {
   RewriterTestContext Context;
   FileID FailingID = Context.createInMemoryFile("invalid/failing.cpp", "test");
   Context.Rewrite.ReplaceText(Context.getLocation(FailingID, 1, 2), 1, "other");
   FileID WorkingID = Context.createOnDiskFile(
     "working.cpp", "line1\nline2\nline3\nline4");
   Context.Rewrite.ReplaceText(Context.getLocation(WorkingID, 2, 1), 5,
                               "replaced");
-  EXPECT_TRUE(Context.Rewrite.overwriteChangedFiles());
+  // This test doesn't create file mappings, so overwriteChangedFiles() needs
+  // no parameter.
+  EXPECT_TRUE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
   EXPECT_EQ("line1\nreplaced\nline3\nline4",
             Context.getFileContentFromDisk("working.cpp")); 
 }
 
 TEST(Rewriter, AdjacentInsertAndDelete) {
   Replacements Replaces;
   auto Err = Replaces.add(Replacement("<file>", 6, 6, ""));
   EXPECT_TRUE(!Err);