[cfe-dev] Locking files on Windows with clangd

[Ilya Biryukov via cfe-dev]

On Wed, Jul 25, 2018 at 8:09 AM Ivan Donchevskii <ivan.donchevskii at qt.io>
wrote:

> There's no way I know to fix it because AFAIK windows always locks mmapped
> files.
>
That's unfortunate, have you seen any pointers of this behavior in official
docs?
I think I was able to modify the mmapped file when I was playing around
with Windows APIs while looking at D35200, but I'll need to rerun my
experiments and make them reproducible.


> 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.
>
I'm not sure there's a single definition of what a "system" header is, e.g.
we pass -isystem when adding include paths to third_party code to suppress
warnings in that code.


> 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 <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 via
> cfe-dev <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
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> --
> Regards,
> Ilya Biryukov
>


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