[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis closed this revision. thakis added a comment. r296408, thanks! https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm We should file a bug against the Rewriter interface. It should really take MemoryBuffers from the caller and handle this for them. https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis added inline comments. Comment at: lib/Rewrite/Rewriter.cpp:455 I->second.write(File.getStream()); + prewrite(I->first); } amccarth wrote: > 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. 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. https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis updated this revision to Diff 89923.
thakis edited the summary of this revision.
thakis added a comment.
Here's a simpler fix, suggested by rnk.
https://reviews.llvm.org/D30385
Files:
tools/clang-format/ClangFormat.cpp
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -236,8 +236,15 @@
// 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 CodeOrErr =
- MemoryBuffer::getFileOrSTDIN(FileName);
+ !OutputXML && Inplace ? MemoryBuffer::getFileAsStream(FileName) :
+ MemoryBuffer::getFileOrSTDIN(FileName);
if (std::error_code EC = CodeOrErr.getError()) {
errs() << EC.message() << "\n";
return true;
@@ -297,9 +304,7 @@
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)
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -236,8 +236,15 @@
// 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 CodeOrErr =
- MemoryBuffer::getFileOrSTDIN(FileName);
+ !OutputXML && Inplace ? MemoryBuffer::getFileAsStream(FileName) :
+ MemoryBuffer::getFileOrSTDIN(FileName);
if (std::error_code EC = CodeOrErr.getError()) {
errs() << EC.message() << "\n";
return true;
@@ -297,9 +304,7 @@
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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
amccarth added inline comments. Comment at: lib/Rewrite/Rewriter.cpp:455 I->second.write(File.getStream()); + prewrite(I->first); } 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. https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
rnk added a comment. 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. https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
Is that a "this patch is good" or a "please rewrite"? On Mon, Feb 27, 2017 at 1:50 PM, Reid Kleckner via Phabricator < revi...@reviews.llvm.org> wrote: > rnk added a comment. > > 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. > > > https://reviews.llvm.org/D30385 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
FILE_SHARE_DELETE is for being able to request a file to be deleted when it's closed, which is too late, right? I didn't write the code in Path.inc, but from what I understand you can't generally overwrite a file that's still open on Windows, and Path.inc tries to work around this -- but the workaround apparently causes the system to leak temp files. rnk will know more about this :-) On Mon, Feb 27, 2017 at 10:10 AM, Manuel Klimek via Phabricator < revi...@reviews.llvm.org> wrote: > klimek added a comment. > > 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... > > > https://reviews.llvm.org/D30385 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis added inline comments. Comment at: include/clang/Rewrite/Core/Rewriter.h:188-197 + /// 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, klimek wrote: > 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? 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. https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
klimek added a comment. 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... https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
klimek added a comment. Can we open the files with FILE_SHARE_DELETE instead? https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
klimek added inline comments. Comment at: include/clang/Rewrite/Core/Rewriter.h:188-197 + /// 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, 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? https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis added a comment. (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) https://reviews.llvm.org/D30385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded
thakis created this revision.
Herald added a subscriber: klimek.
Second attempt after http://llvm.org/viewvc/llvm-project?rev=296166=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. Now, give
`overwriteChangedFiles()` a callback that's called after the input's been read
but before the output's written, and then call `Code.reset()` from that
callback.
(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.)
https://reviews.llvm.org/D30385
Files:
include/clang/Rewrite/Core/Rewriter.h
lib/Frontend/Rewrite/FixItRewriter.cpp
lib/Rewrite/Rewriter.cpp
lib/Tooling/Refactoring.cpp
test/Format/inplace.cpp
tools/clang-format/ClangFormat.cpp
unittests/Tooling/RefactoringTest.cpp
unittests/Tooling/RewriterTest.cpp
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -299,7 +299,10 @@
if (Inplace) {
if (FileName == "-")
errs() << "error: cannot use -i when reading from stdin.\n";
- else if (Rewrite.overwriteChangedFiles())
+ else if (Rewrite.overwriteChangedFiles([, ID](FileID FID) {
+ assert(FID == ID);
+ Code.reset();
+ }))
return true;
} else {
if (Cursor.getNumOccurrences() != 0)
Index: lib/Tooling/Refactoring.cpp
===
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -64,7 +64,9 @@
}
int RefactoringTool::saveRewrittenFiles(Rewriter ) {
- 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(
Index: lib/Frontend/Rewrite/FixItRewriter.cpp
===
--- lib/Frontend/Rewrite/FixItRewriter.cpp
+++ lib/Frontend/Rewrite/FixItRewriter.cpp
@@ -82,9 +82,9 @@
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;
}
Index: lib/Rewrite/Rewriter.cpp
===
--- lib/Rewrite/Rewriter.cpp
+++ lib/Rewrite/Rewriter.cpp
@@ -443,15 +443,16 @@
};
} // end anonymous namespace
-bool Rewriter::overwriteChangedFiles() {
+bool Rewriter::overwriteChangedFiles(llvm::function_ref 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;
Index: include/clang/Rewrite/Core/Rewriter.h
===
--- include/clang/Rewrite/Core/Rewriter.h
+++ include/clang/Rewrite/Core/Rewriter.h
@@ -184,7 +184,18 @@
/// 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 prewrite);
private:
unsigned getLocationOffsetAndFileID(SourceLocation Loc, FileID ) const;
Index: unittests/Tooling/RewriterTest.cpp
===
--- unittests/Tooling/RewriterTest.cpp
+++ unittests/Tooling/RewriterTest.cpp
@@ -19,7 +19,9 @@
