libclang locking files for write/delete on windows

Hello,

I was requested in https://github.com/Valloric/ycmd/issues/61 to post about a file locking problem on windows. In that thread the suspicion was raised that libclang.dll locks header files for writing or delete.

I am writing a plugin that uses YCMD, which uses libclang to provide autocomplete and goto definition among other services.

To be completely honest, libclang and ycmd are a black box for me and I have no idea on their internals, how libclang.dll and YCMD interact or how to start debugging this.
In the above link you can find a .c and .h file that for me consistently lock the files on windows 7 64bit. Only very few header files of my projects get locked, and they all use a code construct as in the above provided .c and .h file. From my experimenting, the header file also seems to must be above a certain size for the lock to occur.

I’m hoping the above is sufficient to pin down the problem.
If more information is need I’m happy to help.

Greetings,

Ivan

So, I think the general problem is that Windows by default does not allow multiple applications to have a file open. The question is whether we could get libclang to read all the files into memory instead of keeping them open…

Wild guess, try this patch:

Index: tools/clang/lib/Lex/HeaderSearch.cpp

Yes and no. Everybody opening a file can specify a share mode, e.g.
"share for read", "share for write" or "exclusive access".

Depending on the share mode for other open handles to the same file,
the next open attempt will fail or succeed.

I don't know if LLVM/Clang or some other app on Ivan's system is
opening the files with exclusive access, but if it's Clang, maybe the
requested share mode could be lowered to read-only?

- Kim

To clarify a bit more when the .h file gets locked and unlocked:

I have 2 processes running:
- sublime text with my plugin
- ycmd with libclang.dll loaded

My sublime text only opens the dummy.c file, it doesn't open dummy.h. It
then tells the ycmd process to parse the dummy.c file.
As soon as that happens, the dummy.h file gets locked. It's either ycmd or
libclang locking it.
When I terminate the ycmd process by hand, the file immediately gets
unlocked, probably windows cleaning up the file handles.

We use this code to open files for reading on Windows:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?view=markup#l756

See the FILE_SHARE_READ | FILE_SHARE_WRITE flags. We could add FILE_SHARE_DELETE and see if that fixes your problem.

Note that locking files against writes or deletions while reading them is pretty common on Windows. MSVC, for example, does this to sources and headers during compilation.

Oh, and here's another one:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?view=markup#l554

It seems to require exclusive access if Mode == readonly, which seems
a little backwards.

This probably isn't used in this code path, though.

- Kim

I’ve experimented a bit with Sysinternals Process Monitor.
The issue seems definetly linked to file size. I took dummy.h and ran it through the monitor. (bad trace)
Then I took dummy.h, removed 90% of the lines from the bottom, and also ran it through the monitor. (good trace)

At some point the traces diverge:
The good trace calls the operations ReadFile, CloseFile and is done.
The bad trace calls CreateFileMapping (which seems to fail with “FILE LOCKED WITH ONLY READERS”), QueryStandardInformationFile, CreateFileMapping, Close File.

So even though the bad trace does seem to close the filehandles, the file remains locked for write or delete.

I’ve also recorded the stack traces of libclang where the two diverge.
badcall.csv: https://drive.google.com/file/d/0B8xFlIzMhHcpZGRpTG9CcDRNSk0/view?usp=sharing

goodcall.csv: https://drive.google.com/file/d/0B8xFlIzMhHcpaEt0UFEzRVJWUjg/view?usp=sharing

You can throw these in your favourite diff tool.

Around some function called llvm MemoryBuffer getOpenFile things start to go different.
I have no experience yet with llvm code base to pinpoint this.

I can also upload the Process Monitor log trace files (*.PML) if someone needs them to analyze.

Hi Ivan,

The issue seems definetly linked to file size. I took dummy.h and ran it through the monitor. (bad trace)
Then I took dummy.h, removed 90% of the lines from the bottom, and also ran it through the monitor. (good trace)
[...]
Around some function called llvm MemoryBuffer getOpenFile things start to go different.
I have no experience yet with llvm code base to pinpoint this.

This is good info; MemoryBuffer::getOpenFile conditionally uses file
mappings or normal file reads depending on file size (see
MemoryBuffer.cpp/shouldUseMmap).

Since the mapping uses a stricter sharing mode than the normal file
read, it's more likely to fail. Still not sure who's writing/deleting
the file while Clang attempts to map it, can you make that out from
your PM logs?

Also, I think I found a bug in MemoryBuffer:

Index: lib/Support/MemoryBuffer.cpp

Good catch Kim, send a patch to cfe-commits.

Hi Ivan,

Hello!

This is good info; MemoryBuffer::getOpenFile conditionally uses file

mappings or normal file reads depending on file size (see
MemoryBuffer.cpp/shouldUseMmap).

Since the mapping uses a stricter sharing mode than the normal file
read, it's more likely to fail. Still not sure who's writing/deleting
the file while Clang attempts to map it, can you make that out from
your PM logs?

I've exported the log to csv and you can find it here:
https://drive.google.com/file/d/0B8xFlIzMhHcpdzR6OW4wM3BZdGM/view?usp=sharing

This is a log of sublime text opening dummy.c, commanding YCMD (python.exe)
to analyze the dummy.c, which so and so on then finally makes libclang.dll
open dummy.h, and finally me killing the python.exe process.
As you can see, killing the proces does not log anything, even though it
"frees" the file.

The trace shows two threads of libclang.dll (TID 6888 and TID 4040), but
they do not operate concurrently. 4040 only starts after 6888 is done with
dummy.h.
When I examine the stack traces of the CreateFileMapping operation that
appears in pairs, they both happen in the same kernel32.dll
CreateFileMappingA function.
So the kernel seems to work around the FILE LOCKED WITH ONLY HEADERS thing
here?

There are no dangling file handles, the trace shows only pairs of open and
close events. This is confirmed by the windows handle.exe tool, which can
not find any filehandles to dummy.h even though the file is locked.
The weird thing is that the windows Process Explorer has a "find file
handle or DLL" function.
When I make it search for dummy.h, it finds not a file handle, but a DLL
inside python.exe called dummy.h.
Somehow Process Explorer reports that dummy.h is loaded as a DLL? (When I
kill python.exe this disappears).

I haven't dived so deep into windows stuff before so I'm hoping this helps!
But to be honest, this is starting to not make sense to me. The file is
locked even though there is no open file handle. Process Explorer reporting
that it is loaded as a DLL...

Regards,

Ivan

Can the memory mapping outlive the file handle? I bet it can.

Hi Reid,

Yep; from Path.inc/mapped_region::init:

  // Close all the handles except for the view. It will keep the other handles
  // alive.
  ::CloseHandle(FileMappingHandle);
  if (FileDescriptor) {
    if (CloseFD)
      _close(FileDescriptor); // Also closes FileHandle.
  } else
    ::CloseHandle(FileHandle);
  return std::error_code();

But as far as I can reason about it, the mapping is unmapped in
~mapped_region. Maybe the MemoryBuffer backed by the mapping is cached
somewhere and lives on?

- Kim

From http://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx

Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order.

Since CloseHandle is already called after the init, it seems the mapped view is still open?
I’m a C and python programmer and (no C++ experience at all) have an incredible hard time reading the code.
Nevertheless, Mapping in mapped_file_region seems to be the only view created and it is unmapped in the destructor:

mapped_file_region::~mapped_file_region() {
if (Mapping)
::UnmapViewOfFile(Mapping);
}

Can I conclude that the destructor is not called?

In normal compilation, the file remains mapped into memory until
compilation is over. Sometimes the AST has pointers to string data embedded
in the code. In YCM, I guess compilation is never "over" so the file stays
mapped indefinitely.

We need to figure out a file mapping strategy that will work on Windows. On
Linux, there are no file locks, and "well behaved" editors generally save
files atomically by writing a temp file and renaming it over the original
file. I don't know what editors on Windows do. If popular editors
(Notepad+, VS, sublime, Vim, etc) rewrite the file in place, then we might
have to give up on the mmap approach, as it will be racy.

mapped_file_region is owned by a MemoryBufferMMapFile (MemoryBuffer.cpp) which inherits from MemoryBuffer.

The MemoryBuffer for a source file lives in a ContentCache, owned by clang::SourceManager::Fileinfos (SourceManager.cpp).

The fileinfos structure is destructed in clang SourceManager destructor which happens when clang completes compiling the compilation unit.

To summarize ~mapped_file_region gets called when clang completes compiling every file.

Before this bug report gets lost, Kim is correct about the error code EC, it’s an output argument for MemoryBufferMMapFile::MemoryBufferMMapFile and must be passed by reference, like:

Index: lib/Support/MemoryBuffer.cpp

In normal compilation, the file remains mapped into memory until
compilation is over. Sometimes the AST has pointers to string data embedded
in the code. In YCM, I guess compilation is never "over" so the file stays
mapped indefinitely.

We need to figure out a file mapping strategy that will work on Windows. On

Linux, there are no file locks, and "well behaved" editors generally save
files atomically by writing a temp file and renaming it over the original
file. I don't know what editors on Windows do. If popular editors
(Notepad+, VS, sublime, Vim, etc) rewrite the file in place, then we might
have to give up on the mmap approach, as it will be racy.

I tried some different editors and commands on the file

Fails to write file: Sublime text 2, notepad++
Can write file: Visual studio 2013, windows notepad

Fails to delete file: explorer.exe, Total Commander
Can delete file: del command on commandline

Funny, but painful, how they all act different.

So if this boils down to libclang keeping this file mapped in memory (with
sharemode to only read) around to provide an AST and functionality to
external tools, maybe the sharemode could be relaxed from read only to
read/write/delete when not actively compiling?
Or maybe the external tool should be able to tell libclang to clean the
mapping up?
I briefly looked at the C api and saw many "dispose" functions. Will
functions like clang_disposeTranslationUnit() not clean up the file mapping?

I hope this is not a too uninformed suggestion!

Hi Yaron,

Before this bug report gets lost, Kim is correct about the error code EC,
it's an output argument for MemoryBufferMMapFile::MemoryBufferMMapFile and
must be passed by reference, like:

Index: lib/Support/MemoryBuffer.cpp

--- lib/Support/MemoryBuffer.cpp (revision 223970)
+++ lib/Support/MemoryBuffer.cpp (working copy)
@@ -203,7 +203,7 @@

public:
   MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t Len,
- uint64_t Offset, std::error_code EC)
+ uint64_t Offset, std::error_code &EC)
       : MFR(FD, false, sys::fs::mapped_file_region::readonly,
             getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC) {
     if (!EC) {

This passes regression tests.

I started trying to create a unit test in
llvm/unittests/Support/MemoryBufferTest.cpp yesterday that proves this
change, but I think I started at the wrong level. I went at trying to
prove that MemoryBuffer::getOpenFile returned an invalid object on
error, but that would mean having to force mapping to fail
(consistently on all platforms).

Maybe it's better to add a test that MemoryBufferMMapFile's
constructor returns the error code through an out arg? Then It should
be trivial to test with an invalid FD or something.

Let me know if you have other ideas, I can try and put a patch together today.

- Kim

Hi Kim,

I think the code you quoted above

std::error_code EC;
std::unique_ptr Result(
new (NamedBufferAlloc(Filename))
MemoryBufferMMapFile(RequiresNullTerminator, FD, MapSize, Offset, EC));
if (!EC)
return std::move(Result);

shows EC is an output variable: it is constructed before MemoryBufferMMapFile() and tested just after. The missing & is probably a typo.

For a unit test your second approach seems better since it directly tests MemoryBufferMMapFile: pass MemoryBufferMMapFile invalid FD or MapSize and test for errors.

Interesting this wen unnoticed. MemoryBufferMMapFile probably gets valid arguments and never fails.

Yaron