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 ↗

(On Diff #89823)

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 ↗

(On Diff #89823)

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 ↗

(On Diff #89823)

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 ↗

(On Diff #89823)

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!

tools/clang-format/ClangFormat.cpp

outs() << "<replacement "
            << "length='" << R.getLength() << "'>";
     outputReplacementXML(R.getReplacementText());
     outs() << "</replacement>\n";
   }
 }
 
 // Returns true on error.
 static bool format(StringRef FileName) {
+  if (!OutputXML && Inplace && FileName == "-") {
+    errs() << "error: cannot use -i when reading from stdin.\n";
+    return false;
+  }
+  // On Windows, overwriting a file with an open file mapping doesn't work,
+  // so read the whole file into memory when formatting in-place.
   ErrorOr<std::unique_ptr<MemoryBuffer>> CodeOrErr =
+      !OutputXML && Inplace ? MemoryBuffer::getFileAsStream(FileName) :
                               MemoryBuffer::getFileOrSTDIN(FileName);
   if (std::error_code EC = CodeOrErr.getError()) {
     errs() << EC.message() << "\n";
     return true;
   }
   std::unique_ptr<llvm::MemoryBuffer> Code = std::move(CodeOrErr.get());
   if (Code->getBufferSize() == 0)
     return false; // Empty files are formatted correctly.
   std::vector<tooling::Range> Ranges;
DiagnosticsEngine Diagnostics(
         IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs),
         new DiagnosticOptions);
     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())
+      if (Rewrite.overwriteChangedFiles())
         return true;
     } else {
       if (Cursor.getNumOccurrences() != 0)
         outs() << "{ \"Cursor\": "
                << FormatChanges.getShiftedCodePosition(CursorPosition)
                << ", \"IncompleteFormat\": "
                << (IncompleteFormat ? "true" : "false") << " }\n";
       Rewrite.getEditBuffer(ID).write(outs());