[MemoryBuffer] Add the setter to be able to force disabled mmap

[yvvan]

Summary

Memory mapping locks files on Windows. Let's give clients an ability to disable mmap if they want to keep all files free of locks.

[yvvan]

VFS is moved to llvm in 8.0, update the diff

[ilya-biryukov]

I don't think isVolatile is the right default at the FileManager level, even on Windows. E.g. memory mapping is probably optimal for compiler, even though it definitely breaks the editors.

but it might be not the only issue.

Could you give a repro that fails? To avoid doing changes we don't fully understand.

[yvvan]

@ilya-biryukov

I have the reported evidence but didn't yet have time to investigate myself.
This is probably caused by our upgrade to Clang-7 which shows that we can't rely on the aimed fixes like the one I mention (D47460).

Another option that I can suggest here is to remove all default isVolatile values at all, track all callers and specify the proper value in each case (ideally depending on the current flags). What do you think?

[ilya-biryukov]

In D54995#1311010, @yvvan wrote:

@ilya-biryukov

I have the reported evidence but didn't yet have time to investigate myself.
This is probably caused by our upgrade to Clang-7 which shows that we can't rely on the aimed fixes like the one I mention (D47460).

Another option that I can suggest here is to remove all default isVolatile values at all, track all callers and specify the proper value in each case (ideally depending on the current flags). What do you think?

Before we start changing all the callers or the defaults, it would be nice to know we don't have other options. The reason I'm begging for a repro is to understand what's going and make sure we can't fix it on the lower levels.
If we want to switch everyone to use non-memory-mapped files by default, we better have a very good reason to do so and know about the trade-offs.

My hope is that we can get rid of this flag some day rather than put it into more places or change a default to something that seems less efficient (I don't have data, so that's an uninformed opinion).

[yvvan]

I've realized that this patch covers too much stuff outside of clang and I have no idea how bad is to not memory map it.

"My hope is that we can get rid of this flag some day" - i'm not sure it's possible. For that we need some concept of user and system files in llvm similar to clang::SrcMgr::CharacteristicKind. But we can probably make an adapter for the MemoryBuffer in clang which will do the work at least in clang case.

[yvvan]

Hm, probably FileManager can be that adapter since it's in clang

[ilya-biryukov]

cross-posting @zturner's comment from the mailing thread for the record:

I’m afraid this is going to be too severe a performance regression if we change the default. So I don’t think this should be an LLVM-wide default

And yet, before proceeding further with this, could we figure out the exact cause of the issue? I.e.

• What are the combination of file open flags that force the files to be locked on windows?
• Is there a way to memory map a file on Windows without locking a file?
• What is the sequence of actions that trigger this in clang? Is it hard to notice because we keep files mmapped for small periods of time?
• Can we try to make a reliable repro? Even if it's infrequent, there are probably various hacky ways to make it more reproducible, e.g. adding sleeps into various points to keep the files locked for longer.

[yvvan]

"could we figure out the exact cause of the issue?"

I'm pretty sure we can. Currently the issue is mentioned in our bugreports (https://bugreports.qt.io/secure/attachment/78582/clang-locks.png) and i plan to reproduce it and track the exact issue.
And this review was not meant to proceed immediately but rather state the intention and get some feedback because I still don't know answers to the questions 1 and 2 (did somebody else investigate that?)

[yvvan]

@ilya-biryukov

According to https://stackoverflow.com/a/7414798 (if it's true) we can't prevent locks.

[ilya-biryukov]

In D54995#1311303, @yvvan wrote:

"could we figure out the exact cause of the issue?"
And this review was not meant to proceed immediately but rather state the intention and get some feedback because I still don't know answers to the questions 1 and 2 (did somebody else investigate that?)

I tried calling Windows APIs that LLVM uses with multiple combinations of flags and couldn't make it lock the file. But maybe I didn't arrive at the right combination of flags or was trying on a different version.

Could you send a link for the issue itself, I'm not sure how to get from the attachment to an actual issue, would be interested to look at this

[yvvan]

@ilya-biryukov

https://bugreports.qt.io/browse/QTCREATORBUG-15449

I was able to lock files last time with mmap. The Windows API calls are CreateFileMappingW (it's in the Path.inc, line 794) and MapViewOfFile (further down). As far as I remember the second call actually creates the lock and is freed only by UnmapViewOfFile in destructor

[zturner]

In D54995#1311399, @ilya-biryukov wrote:

In D54995#1311303, @yvvan wrote:

"could we figure out the exact cause of the issue?"
And this review was not meant to proceed immediately but rather state the intention and get some feedback because I still don't know answers to the questions 1 and 2 (did somebody else investigate that?)

I tried calling Windows APIs that LLVM uses with multiple combinations of flags and couldn't make it lock the file. But maybe I didn't arrive at the right combination of flags or was trying on a different version.

Could you send a link for the issue itself, I'm not sure how to get from the attachment to an actual issue, would be interested to look at this

My first thought would be that it depends on the flags to CreateFile moreso (and perhaps entirely) rather than the flags to MapViewOfFile or CreateFileMapping. Specifically, the FILE_SHARE_XXX flags are the most relevant here.

[ilya-biryukov]

In D54995#1311427, @zturner wrote:

My first thought would be that it depends on the flags to CreateFile moreso (and perhaps entirely) rather than the flags to MapViewOfFile or CreateFileMapping. Specifically, the FILE_SHARE_XXX flags are the most relevant here.

That was my initial thought as well and it worked fine on Windows Server 2012. Will have to do it again to double-check, though, not sure I can dig up that code.

In D54995#1311419, @yvvan wrote:

https://bugreports.qt.io/browse/QTCREATORBUG-15449
I was able to lock files last time with mmap. The Windows API calls are CreateFileMappingW (it's in the Path.inc, line 794) and MapViewOfFile (further down). As far as I remember the second call actually creates the lock and is freed only by UnmapViewOfFile in destructor

Thanks. I think I did the same on Windows Server 2012 and it didn't lock the files. I'll try to repeat my experiments.

[yvvan]

@ilya-biryukov

To clarify a little bit - i did not write my own simple app but used libclang in qt creator to reproduce the lock and track the issue. This time I hope to go further, collect flags used by clang and make a simple example.

[yvvan]

According to https://msdn.microsoft.com/en-us/2e9c3174-af48-4fa3-9f6a-fb62b23ed994 - "Unmapping a mapped view of a file invalidates the range occupied by the view in the address space of the process and makes the range available for other allocations".
Also as far as i understand from https://msdn.microsoft.com/en-us/library/ms810613.aspx mapped files can only be edited in other apps as mapped files opened with the same name (OpenFileMapping).

Simple example, I launch it and until i enter the number the test.h header remains locked and therefore i can't edit it in other apps.

DWORD shareMode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
int accessRights = 0;
accessRights |= GENERIC_READ;
SECURITY_ATTRIBUTES securityAtts = { sizeof(SECURITY_ATTRIBUTES), NULL, FALSE };
HANDLE fileHandle =
    CreateFile("D:\\test.h", accessRights, shareMode, &securityAtts,
               OPEN_EXISTING,
               FILE_FLAG_BACKUP_SEMANTICS,
               NULL);

HANDLE FileMappingHandle =
    CreateFileMappingA(fileHandle, 0, PAGE_READONLY, 0, 0, "D:_code_test.h");

LPVOID Mapping = MapViewOfFile(FileMappingHandle, FILE_MAP_READ, 0, 0, 0);
assert(Mapping);

CloseHandle(FileMappingHandle);

int i {0};
std::cin >> i;
UnmapViewOfFile(Mapping);

CloseHandle(fileHandle);

[lebedev.ri]

Passing-by thought, feel free to ignore: this seems to so far only affect windows only?
So the fix shouldn't probably pessimize all other arches? (and maybe even non-clangd)

[yvvan]

I'm currently trying out another suggestion - which unmaps memory buffer caches after ASTUnit's Parse or Reparse and is limited to Windows only.
And my aim currently is not only clangd but any other client as well.

[yvvan]

No success with unmapping. Buffers are hold by PCMCache in Preprocessor (and the same one in ASTReader) and if I clean them then SourceManger crashed sooner or later on some call that gets data from external files.
So far I have no steps to reproduce the lock on user file so I don't currently know if I try something else.

[ilya-biryukov]

In D54995#1312617, @lebedev.ri wrote:

Passing-by thought, feel free to ignore: this seems to so far only affect windows only?
So the fix shouldn't probably pessimize all other arches? (and maybe even non-clangd)

Sure, we'll make sure to keep the compiler and libclang/clangd on non-windows archs still use memory-mapped files.
I even have hopes there are ways to workaround the locks on Windows, but I'm probably too optimistic.

In D54995#1312532, @yvvan wrote:

According to https://msdn.microsoft.com/en-us/2e9c3174-af48-4fa3-9f6a-fb62b23ed994 - "Unmapping a mapped view of a file invalidates the range occupied by the view in the address space of the process and makes the range available for other allocations".
Also as far as i understand from https://msdn.microsoft.com/en-us/library/ms810613.aspx mapped files can only be edited in other apps as mapped files opened with the same name (OpenFileMapping).

I couldn't find any reference to file locking in those docs (I haven't thoroughly read the second one, though, so I might've missed it). Will check out your example program, thanks for posting it!

In D54995#1314133, @yvvan wrote:

No success with unmapping. Buffers are hold by PCMCache in Preprocessor (and the same one in ASTReader) and if I clean them then SourceManger crashed sooner or later on some call that gets data from external files.
So far I have no steps to reproduce the lock on user file so I don't currently know if I try something else.

Unmapping definitely won't work, there are a ton of operations that might require the file contents. I don't think we can make clang code avoid reading the files it previously had opened.
If no workaround works, reading files on windows instead of memory-mapping them is probably the only option for interactive editors.

[zturner]

Original patch description says this:

There are reported cases of non-system files being locked by libclang on Windows (and likely by other clients as well)

What is the nature of the reports? What operation is attempted on the files and fails due to locking? And which application is it that's failing and in what way?

[ilya-biryukov]

In D54995#1314912, @zturner wrote:

Original patch description says this:

There are reported cases of non-system files being locked by libclang on Windows (and likely by other clients as well)

What is the nature of the reports? What operation is attempted on the files and fails due to locking? And which application is it that's failing and in what way?

https://bugreports.qt.io/browse/QTCREATORBUG-15449 was mentioned previously in the thread.

I've tried reproducing with the provided example and succeeded: if I map a view of the file, none of the editors would be able to write into that file.
I tried various combinations of flags, trying really hard to do readonly and copy-on-write mapping, but none would work, a file cannot be written into while there are active mapped views of the it.

E.g. for VSCode process monitor reports it eventually tries to call SetEndOfFile and fails with ERROR_USER_MAPPED_FILE error (it can be found in the list of errors from Microsoft).
There's this also Stackoverflow question mentioning some cases where this occurs.

It looks like there's no workaround for this case in the Windows APIs, so I guess we'll have to trade-off memory in that case and read the whole file using the classical file APIs.

@yvvan, I was proven wrong. Thanks for taking your time to go through the trouble of providing this example. I'm not sure why my previous experiments came up with a different result, I've probably made a mistake somewhere along the way.
Nevertheless, could we avoid changing the defaults to true? We probably forgot to propagate the flag somewhere, those places should be easy to find by temporarily removing the default args, rerunning the build for libclang and then bringing the default args back.
It's fine to enable this behavior for editors, but various command-line tools, including the compiler, are definitely better off using the memory-mapped files.

[zturner]

We can work around it by reading the whole file, but it looks like a bug in QtCreator to me. We have the file mapped, but maybe when they open the file to save it, *they* are opening the file without FILE_SHARE_READ. It's a logical thing to do on the surface, because they are probably thinking "we are about to change the contents of the file, so we don't want anyone to be reading it at the same time while we modify it". But this means their call to CreateFile() will fail, because we have the file opened for read, and if they want to open it for exclusive access, it's not possible.

[ilya-biryukov]

In D54995#1315024, @zturner wrote:

We can work around it by reading the whole file, but it looks like a bug in QtCreator to me. We have the file mapped, but maybe when they open the file to save it, *they* are opening the file without FILE_SHARE_READ.

I haven't tried QtCreator, but this also happens in VSCode and Vim. I suspect other editors are also affected, but I haven't checked.
I looked at the logs in Process Monitor and I think they do set the share flags properly (~95% certain, will still have to double-check). Even if they do it wrong, it will take time to fix this and I don't think we can afford breaking users with existing editors in our tooling.

That being said, I don't think the "isVolatile" parameter should be present in the filesystem APIs. I propose to handle that at the vfs::FileSystem implementation itself that is used underneath.
D55139 is an initial fix for clangd that wraps existing RealFileSystem to force all files being open as volatile. It's much more reliable than passing the flags around, but it has a downside of also not memory-mapping all files, not just user files. However, I would not bother to try saving those bits unless we know that can actually critical for performance or memory usage (which I doubt would matter in the editors use-case, given that we still need to run the compiler on the files that we've read).

[yvvan]

@ilya-biryukov
What do you think about the global settable bool in MemoryBuffer in place of the ifdef from https://reviews.llvm.org/D35200 ? In this case the client on Windows can set it and you're safe that any MemoryBuffer call never mmaps.

[yvvan]

Instead of forcing some default value let's give the client an ability to force disabled mmap if one desires.

[lebedev.ri]

lib/Support/MemoryBuffer.cpp

42 ↗

(On Diff #176318)

Such global flags are a bad idea in general, and really not great in LLVM's case.
The editor would set it for "it's" llvm, but that will also affect the LLVM that is used by e.g. mesa.

[yvvan]

lib/Support/MemoryBuffer.cpp

42 ↗

(On Diff #176318)

Oh no, don't mention mesa. The proper client should never share it's LLVM layer with mesa. We already got issues with versions incompatibility and the only good solution is to link llvm statically inside client. Otherwise mesa causes the mess anyways.

Also I expect this setter to be used only on Windows.

Of course there's another solution is that Ilya proposed but it's only for clangd there and is bigger in code size. And it can still allow bugs if somebody uses MemoryBuffer not through the FileSystem class.

[ilya-biryukov]

It seems there are still cases where memory-mapping should be preferred, even on Windows, specifically:

• PCH files produced by libclang: they are huge, loading them in memory is wasteful and they can clearly be used
• (maybe?) PCM (precompiled module) files: they are huge, however can be produced by the user's buildsystem. Locking them might be bad if we don't control the buildsystem, but the memory savings are attractive.
• other binary input files?

At least the PCH files seem to be low-hanging fruits, we should try to take advantage of them.

lib/Support/MemoryBuffer.cpp

42 ↗

(On Diff #176318)

+1, please don't add global flags, there are lots of reasons to avoid them.
Moreover, adding this without removing the current approach (isVolatile) is twice as bad - we now have many ways to indicate the files cannot be memory-mapped.

[yvvan]

Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?

[ilya-biryukov]

In D54995#1316437, @yvvan wrote:

Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?

Would be happy to, will need to figure out what to do with PCH and PCM files first. However if we do this on clang level, I believe we should remove the isVolatile flag from the VFS interfaces in the first place.
It would be nice to not loose out on the opportunity to avoid fully loading the PCH files, but that obviously requires passing some flags into the VFS implementation or various hacks (matching on filenames/extensions?) to find out which files are PCHs.
I actually don't know which approach to choose: on one hand, I'd really want to get rid of the isVolatile flag, on the other hand I'd really want to avoid loading large binary files into memory and that requires passing the flags.

[yvvan]

In D54995#1316457, @ilya-biryukov wrote:

In D54995#1316437, @yvvan wrote:

Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?

Would be happy to, will need to figure out what to do with PCH and PCM files first. However if we do this on clang level, I believe we should remove the isVolatile flag from the VFS interfaces in the first place.
It would be nice to not loose out on the opportunity to avoid fully loading the PCH files, but that obviously requires passing some flags into the VFS implementation or various hacks (matching on filenames/extensions?) to find out which files are PCHs.
I actually don't know which approach to choose: on one hand, I'd really want to get rid of the isVolatile flag, on the other hand I'd really want to avoid loading large binary files into memory and that requires passing the flags.

I don't think removing the flag is a good idea since I can easily assume cases when user wants mmap and is ready to encounter locks. In our case it can be an IDE option which behavior to choose.
The ideal solution in my opinion would be to have isSystem(filename) in FileSystem class. And is based on system directories with which ASTUnit feeds the FileSystem for example.

[ilya-biryukov]

In D54995#1316476, @yvvan wrote:

I don't think removing the flag is a good idea since I can easily assume cases when user wants mmap and is ready to encounter locks. In our case it can be an IDE option which behavior to choose.

Would that option be useful if changing it forces the files user is editing in the IDE to be indefinitely locked and stops them from saving the files?
Anyway, even if you feel this behavior should be configurable, that's totally fine - I was referring to removing the flag from the vfs::FileSystem interface, possibly replacing it with a filesystem implementation that would handle the same thing at a lower level.
This flag is not useful to any of the filesystem implementation, except RealFileSystem, which actually has to deal with opening OS files.

The ideal solution in my opinion would be to have isSystem(filename) in FileSystem class. And is based on system directories with which ASTUnit feeds the FileSystem for example.

I would disagree that isSystem() is a property of the filesystem. The filesystem abstraction is responsible for providing access to the files and their contents, why should classification of the files be filesystem-dependent? It's the wrong layer for the isSystem() check.
E.g. imagine implementing a filesystem that gets files from the network. The concerns filesystem is responsible for clearly include doing network IO. But why should the system/user classification of the files be different for it? What policy decisions does it have to make there? Clearly some other should do this classification and it's clearly independent of the filesystem being used.
OTOH, passing whether we want to open the files as memory-mapped does not sound completely crazy. But it would still be nice to avoid having it in the interface, since we only need it to workaround limitations specific to Windows + interactive editors, which is only a small subset of LLVM-based tools uses.

[yvvan]

@ilya-biryukov

Hm. What about another way around? - We have user include paths (-I) and report them to the filesystem. This means that we have specific paths under which nothing can be mmaped and everything else can be. In particular cases we can also report -isystem there. This is quite the same logic as current isVolatile parameter but is set only once for each path.

[zturner]

Assuming this patch were to go in as-is (which it probably won't, based on the feedback, but let's just pretend), that would avoid having to explicitly update how many callsites?

What I'm wondering is, how hard would it be to just update the call-sites?

It looks like what you are really trying to do is avoid having to pass "IsVolatile" for many call-sites. But to be honest, IsVolatile exactly describes this situation. You have a file, and another program has the same file and it can change it out from underneath you. That exactly describes the meaning of IsVolatile. So, conceptually it makes sense to just find all the call-sites where this matters and pass IsVolatile=true. Is there some reason we can't just do that here? Are there too many?

[yvvan]

@zturner

The fact that so many call-sites decide what to do is pretty error-prone.
There was already at least one issue when this flag was not properly set by some of the call-sites.

include/llvm/Support/MemoryBuffer.h

   /// while the user is editing/updating the file or if the file is on an NFS.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFile(const Twine &Filename, int64_t FileSize = -1,
-          bool RequiresNullTerminator = true, bool IsVolatile = false);
+          bool RequiresNullTerminator = true, bool IsVolatile = true);
 
   /// Read all of the specified file into a MemoryBuffer as a stream
   /// (i.e. until EOF reached). This is useful for special files that

   /// Since this is in the middle of a file, the buffer is not null terminated.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getOpenFileSlice(int FD, const Twine &Filename, uint64_t MapSize,
-                   int64_t Offset, bool IsVolatile = false);
+                   int64_t Offset, bool IsVolatile = true);
 
   /// Given an already-open file descriptor, read the file and return a
   /// MemoryBuffer.

   /// while the user is editing/updating the file or if the file is on an NFS.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getOpenFile(int FD, const Twine &Filename, uint64_t FileSize,
-              bool RequiresNullTerminator = true, bool IsVolatile = false);
+              bool RequiresNullTerminator = true, bool IsVolatile = true);
 
   /// Open the specified memory range as a MemoryBuffer. Note that InputData
   /// must be null terminated if RequiresNullTerminator is true.

tools/clang/include/clang/Basic/FileManager.h

   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
+  getBufferForFile(const FileEntry *Entry, bool isVolatile = true,
                    bool ShouldCloseOpenFile = true);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFile(StringRef Filename, bool isVolatile = false);
+  getBufferForFile(StringRef Filename, bool isVolatile = true);
 
   /// Get the 'stat' information for the given \p Path.
   ///

tools/clang/include/clang/Basic/VirtualFileSystem.h

   /// Get the contents of the file as a \p MemoryBuffer.
   virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize = -1,
-            bool RequiresNullTerminator = true, bool IsVolatile = false) = 0;
+            bool RequiresNullTerminator = true, bool IsVolatile = true) = 0;
 
   /// Closes the file.
   virtual std::error_code close() = 0;

   /// closes the file.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(const Twine &Name, int64_t FileSize = -1,
-                   bool RequiresNullTerminator = true, bool IsVolatile = false);
+                   bool RequiresNullTerminator = true, bool IsVolatile = true);
 
   /// Get a directory_iterator for \p Dir.
   /// \note The 'end' iterator is directory_iterator().

tools/clang/lib/Basic/VirtualFileSystem.cpp

   ErrorOr<std::unique_ptr<MemoryBuffer>> getBuffer(const Twine &Name,
                                                    int64_t FileSize,
                                                    bool RequiresNullTerminator,
-                                                   bool IsVolatile) override;
+                                                   bool IsVolatile = true) override;
   std::error_code close() override;
 };