Use volatile filesystem on Windows

[Crunkle]

When Clang loads up the files required for indexing it memory maps any source files over a specific threshold. This is fine for Unix, but the strict file locking policies on Windows cause issues especially when an IDE saves the file and triggers a textDocument/didSave. If the file is re-saved before the previous indexing has complete, the save will fail and often produce various ambiguous errors.

I have been using this for a month and I believe clangd has something very similar implemented. No issues so far, even on very large source files, so I want to merge if possible.

[MaskRay]

Thanks for looking into the issue! It is probably #235 (CC @Riatre)

When Clang loads up the files required for indexing it memory maps any source files over a specific threshold.

To expand this topic a bit, clang::FrontendAction::BeginSourceFile creates a SourceManager with UserFilesAreVolatile set to true if it doesn't exist:

// lib/Frontend/FrontendAction.cpp#L673
  if (!CI.hasSourceManager())
    CI.createSourceManager(CI.getFileManager());

// lib/Frontend/CompilerInstance.cpp
void CompilerInstance::createSourceManager(FileManager &FileMgr) {
  SourceMgr = new SourceManager(getDiagnostics(), FileMgr); // third arg defaults to false
}

This is fine for Unix, but the strict file locking policies on Windows cause issues especially when an IDE saves the file and triggers a textDocument/didSave. If the file is re-saved before the previous indexing has complete, the save will fail and often produce various ambiguous errors.

I don't use Windows and am not clear what Windows does. If my understanding is correct, we need a fix similar to d487120. That commit fixed sema_manager.cc (was clang_complete.cc; it implements completion and diagnostics) but we probably need a fix in indexer.cc (the component that indexes a file. The index will serve various textDocument/{definition,references,etc} $ccls/member etc requests).

I have been using this for a month and I believe clangd has something very similar implemented. No issues so far, even on very large source files, so I want to merge if possible.

If you can reliably reproduce this issue. Would you mind trying the following patch?
It should fix the same issue but the change is much less intrusive.

diff --git i/src/indexer.cc w/src/indexer.cc
index 280e3c4d..69eb7291 100644
--- i/src/indexer.cc
+++ w/src/indexer.cc
@@ -1263,6 +1263,8 @@ Index(SemaManager *manager, WorkingFiles *wfiles, VFS *vfs,
   Clang->setVirtualFileSystem(FS);
   Clang->createFileManager();
 #endif
+  Clang->setSourceManager(new SourceManager(Clang->getDiagnostics(),
+                                            Clang->getFileManager(), true));
 
   IndexParam param(*vfs, no_linkage);
   auto DataConsumer = std::make_shared<IndexDataConsumer>(param);

I have been using this for a month and I believe clangd has something very similar implemented.

clangd doesn't seem to call setSourceManager(). In addition, PrecompiledPreamble::Build used by both projects has:

  // Create the source manager.
  Clang->setSourceManager(
      new SourceManager(Diagnostics, Clang->getFileManager()));

It may very unlikely cause an issue, though.

[Crunkle]

That is great, thanks! I was looking at the best way to set UserFilesAreVolatile but totally overlooked reconstructing the source manager. I have now tested your solution today and it has completely resolved all of my issues. It's far more elegant than my previous commit so I agree, we should definitely use your patch instead.

As for precompiled headers: I did see that clangd makes an exception for them but I don't think there is any valid rationale for us to do the same. Your patch fixes write locks, especially after saving the file, and so precompiled shouldn't cause any problems.

I have updated my pull request but feel free to close and push whatever you think is best. You are more familiar with this than me but I will be very happy to see this resolved.

[MaskRay]

@Crunkle Can you confirm that without the fix, you'll see issues like #235?

As for precompiled headers: I did see that clangd makes an exception for them but I don't think there is any valid rationale for us to do the same. Your patch fixes write locks, especially after saving the file, and so precompiled shouldn't cause any problems.

Can you elaborate the exception?

UserFilesAreVolatile=true (introduced in rC160074) is not very ideal. It doesn't protect system headers and included filed via -isystem.

[Crunkle]

Yes, without the fix I have various issues all somehow related to acquiring a write lock. Similarly, I have no issues with the fix and the debugger shows all is working correctly with no memory maps.

Can you elaborate the exception?

See clangd's FSProvider.cpp:39. This is the check to skip generated premables as they are building them themselves. That may be more optimal but I do not think it is necessary to do so.

Also note clang's Basic/SourceManager.cpp:137.