Windows Rewriter/MemoryBuffer errors with 16KB large files

[llvmbot]

Bugzilla Link	17960
Version	trunk
OS	Windows NT
Reporter	LLVM Bugzilla Contributor
CC	@rnk

Extended Description

Various parts of tooling, the modernizers and migrators are mis-using the Rewriter interface and causing Windows errors or crashes when rewriting large files.

A typical error:

"Error while writing file: Error opening output file \'1.cpp\': The requested operation cannot be performed on a file with a user-mapped section open"

The problematic pattern appearing frequently in clang's codebase is:

  llvm::raw_fd_ostream FileStream(FileName.c_str(), ErrorInfo,
                                  llvm::sys::fs::F_Binary);
  Rewrite.getEditBuffer(ID).write(FileStream);

The fix is often simple as just replacing with:

  Rewrite.overwriteChangedFiles()

overwriteChangedFiles() knows how to write to a temporary file and move into place without writing over the mmaped source MemoryBuffer.

My analysis of the underlying problem:

The previous implementation was attempting to stream back directly to the
original file and failing if it was already memory mapped by MemoryBuffer,
an operation unsupported by Windows.

MemoryBuffer generally mmaps files larger than the physical page.

I've so far fixed two instances of the problem:

• r193594: RefactoringTool
• r194250: clang-format (#17216 )

There are a others yet to be fixed, some more involved, which I'm filing this bug to track.

The clang lit tests have mostly missed this so far because there's a tendency to keep test inputs checked into version control small, but in practice this gets hit often when running the rewrite-based tools on Windows with real code.

[rnk]

I fixed an instance of this in r239920.

[llvmbot]

A note on debugging:

The file size varies by OS version, from 4KB (XP), to 16KB on Windows 8 to 1MB (some configurations of Windows Server I believe).

So the easy way to debug the problem and identify remaining problems is to force shouldUseMmap() to always return true.

(And the easy way to work around the problem is to force the same function to always return false.)

[llvmbot]

Although it's not technically broken, I'm going to propose making RewriteBuffer::write(raw_ostream &Stream) a protected member to encourage callers to migrate to Rewriter::overwriteChangedFiles() in the post-3.4 timeframe. Realistically most people don't have a Windows machine around to test and don't need to know about the details of Windows mmap so it's a good idea to make the interface defensive to porting issues like this.

As for the 3.4 release, the only quick fix I can think of (which has been successfully deployed in the unofficial pre-release builds) is to disable LLVM MemoryBuffer's memory mapped file feature completely on Windows, pending fixes for the remaining callers. This gets all the tooling and rewrite-based utilities more or less working and we've been able to use them in production with the workaround applied.

[Endilll]

I wonder if migration to Rewriter::overwriteChangedFiles() has already happened, or we addressed this issue some other way.