[cfe-dev] Locking files on Windows with clangd

Tue Jul 24 23:09:12 PDT 2018

Hi!

Yes, we also still have that issue on Windows and have some of our own bug reports (https://bugreports.qt.io/browse/QTCREATORBUG-15449)

For some time we used the workaround  from D35200 but abandoned it finally because of the complaints about high memory consumption.


There's no way I know to fix it because AFAIK windows always locks mmapped files. But i can't say that this behavior is broken because you are not supposed to mark file 'system' if you plan to modify it. What would I do if I had bugreport with '#pragma system_header' - I would simply remove that line from the unsaved file content I pass to clang.


Regards,

Ivan

________________________________
From: Ilya Biryukov <ibiryukov at google.com>
Sent: Tuesday, July 24, 2018 9:13:47 AM
To: Dmitry.Kozhevnikov at jetbrains.com; Ivan Donchevskii
Cc: via cfe-dev
Subject: Re: [cfe-dev] Locking files on Windows with clangd

Hi Dmitry,

We've had a pull request with a similar fix at some point: https://reviews.llvm.org/D35200, but it was abandoned and never made it in.
@ivan.donchevskii at qt.io<mailto:ivan.donchevskii at qt.io>, did you manage to figure out a way to make this work on Windows? Any ideas on what's the best way to fix this?


On Sun, Jul 22, 2018 at 7:22 PM Dmitry.Kozhevnikov at jetbrains.com<mailto:Dmitry.Kozhevnikov at jetbrains.com> via cfe-dev <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
Hi!

A short background: we at JetBrains are experimenting with using clangd with
CLion, and we're very happy with the results so far. Unfortunately, we've hit
a problem when header files are locked on Windows, so they can't be saved,
they break `git rebase` (leading to the loss of work), etc.

It happens when the file is opened using a memory mapped file. It's fairly
easy to reproduce with clangd:

1. Create a header big enough to pass the threshold in `shouldUseMmap()`
(lib/Support/MemoryBuffer.cpp)
2. Include it in a source file
3. Keep this source file opened via clangd
4. Now an attempt to modify this header will fail

The situation is especially unpleasant because the handle is locked not only
during the parse, but for all the time clangd holds the preamble containing
this header.

This issue is mitigated to an extent in libclang: non-system files are
considered "volatile" (see r160074, r334070), so memory mapped files are not
used for them. However, I feel like it's not enough: you can easily have a
`#pragma system_header` in your codebase (IIUC it affects the mentioned
behavior), locking such file and i.e. losing user's work during `git rebase`
is still unacceptable.

Also, I think the fact that the proper compiler has this behavior is also
unfortunate (forgotten build in the background might lead to the loss of data).

IIUC the main reason for having memory mapped files is to reduce the memory
footprint. However, for a regular translation unit, headers usually take several
megabytes, which IMO is tolerable, and is usually topped by other per-TU data
anyway.

Hence, what do you think about not using memory mapped files on Windows at all
for source files? Are there any implications that I'm not aware of?

The naive patch (obviously not for commit, just for the illustration):

--- a/lib/Basic/SourceManager.cpp
+++ b/lib/Basic/SourceManager.cpp
@@ -109,7 +109,12 @@ llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
return Buffer.getPointer();
}

+#ifndef _WIN32
bool isVolatile = SM.userFilesAreVolatile() && !IsSystemFile;
+#else
+ bool isVolatile = true;
+#endif
+
auto BufferOrError =
SM.getFileManager().getBufferForFile(ContentsEntry, isVolatile);

If I've not missed something, this would effectively disable memory mapped
files for source files on Windows, but they would still be used for
other potentially large files (including PCHs and preambles).

If this is unacceptable, at the very least, clangd should treat user files
similar to libclang (we can work on a patch in this case).

Note that there is a similar bug report for QtCreator/libclang with some
interesting comments: https://bugreports.qt.io/browse/QTCREATORBUG-15449

Cheers,
Dmitry.
_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180725/a2ada7fb/attachment.html>
