[cfe-dev] Locking files on Windows with clangd

Wed Jul 25 00:01:10 PDT 2018

Hi, Ivan!

Thank you for the insights!

> 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.

> There are different ways to suppress warnings other than passing -isystem so we're trying to make fewer -isystem paths

Not sure I can agree with that:

- whether a directory is marked w/ `-isystem` or not is the user's/buildsystem decision, not the IDE's, so we can't say to the user "just don't mark directories you want to modify with `-isystem` - it looks like a poor way do deal with Windows locking issues.
- locking "proper system" files also seems suboptimal, as it e.g. would break package managers updates

>  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.

It also applies to the transitively included headers - it would be really unfortunate to do the same for all of them

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

Have you ever managed to pinpoint the problem? I would expect that the issue with D35200 would be keeping preambles in memory, not source files, but it would be great to have a conformation or a contradiction.

> On 25 Jul 2018, at 09:31, Ivan Donchevskii <ivan.donchevskii at qt.io> wrote:
> 
> There are different ways to suppress warnings other than passing -isystem so we're trying to make fewer -isystem paths (for example https://reviews.llvm.org/D48116 which i really like and am planing to update in the nearest future)
> From: Ilya Biryukov <ibiryukov at google.com>
> Sent: Wednesday, July 25, 2018 8:20:40 AM
> To: Ivan Donchevskii
> Cc: Dmitry.Kozhevnikov at jetbrains.com; via cfe-dev
> Subject: Re: [cfe-dev] Locking files on Windows with clangd
>  
> 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, 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 --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180725/5892bd56/attachment.bin>