Fix sourcekitd persistent file-locks on Windows

[z2oh]

Fixes swiftlang/sourcekit-lsp#644

During semantic analysis, swift::vfs::getFileOrSTDIN opens files with a default IsVolatile=false arg, which makes its way down through MemoryBuffer::getOpenFile, to shouldUseMmap. If this function's heuristics returned true, the file would be mmapped by sourcekitd and then could no longer be saved in an open editor until sourcekitd was killed. One of the heuristics used is a file-size check to avoid memory-mapping small files, and so this issue would only trigger on large source files.

This patch propagates LangOpts.DiagnosticsEditorMode into SourceManager as a new editorMode boolean, that is now passed as the IsVolatile parameter to swift::vfs::getFileOrSTDIN, avoiding memory mapping files when the compiler is being invoked by sourcekitd.

Stack trace for the curious:

00 00000061`50af9df8 00007ff9`9708b391     KERNELBASE!wil::details::DebugBreak+0x2
01 00000061`50af9e00 00007ff9`9708d121     sourcekitdInProc!llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer,std::default_delete<llvm::MemoryBuffer> > >::ErrorOr<std::unique_ptr<llvm::MemoryBuffer,std::default_delete<llvm::MemoryBuffer> > ><std::unique_ptr<llvm::WritableMemoryBuffer,std::default_delete<llvm::WritableMemoryBuffer> > >+0x741
02 00000061`50af9f10 00007ff9`970d5a8a     sourcekitdInProc!llvm::MemoryBuffer::getOpenFile+0x41
03 00000061`50af9f70 00007ff9`970d5c47     sourcekitdInProc!llvm::vfs::Status::exists+0x15a
04 00000061`50af9fc0 00007ff9`9df9e55a     sourcekitdInProc!llvm::vfs::FileSystem::getBufferForFile+0x197
05 00000061`50afa050 00007ff9`9df93901     sourcekitdInProc!swift::vfs::getFileOrSTDIN+0x11a
06 00000061`50afa240 00007ff9`9b0a3e03     sourcekitdInProc!swift::SourceManager::getExternalSourceBufferID+0xe1
07 00000061`50afa2e0 00007ff9`9b09ac13     sourcekitdInProc!swift::Decl::getSerializedLocs+0x143
08 00000061`50afa530 00007ff9`9ab0b4e5     sourcekitdInProc!swift::Decl::getLoc+0x153
09 00000061`50afa560 00007ff9`9ab0d613     sourcekitdInProc!llvm::TinyPtrVector<swift::AssociatedTypeDecl * __ptr64>::push_back+0xe35
0a 00000061`50afa880 00007ff9`9aaffdcb     sourcekitdInProc!llvm::PointerIntPair<swift::Type,2,unsigned int,llvm::PointerLikeTypeTraits<swift::Type>,llvm::PointerIntPairInfo<swift::Type,2,llvm::PointerLikeTypeTraits<swift::Type> > >::setPointerAndInt+0x10d3
0b 00000061`50afb290 00007ff9`9a7b91ea     sourcekitdInProc!swift::ResolveTypeWitnessesRequest::evaluate+0xcb
0c 00000061`50afb970 00007ff9`9a85969c     sourcekitdInProc!swift::SimpleRequest<swift::ResolveTypeWitnessesRequest,std::tuple<> __cdecl(swift::NormalProtocolConformance * __ptr64),2>::evaluateRequest+0x1a
0d 00000061`50afb9a0 00007ff9`9a858958     sourcekitdInProc!swift::Evaluator::getResultUncached<swift::ResolveTypeWitnessesRequest,`swift::evaluateOrDefault<swift::ResolveTypeWitnessesRequest>'::`2'::<lambda_1> >+0x1fc
0e 00000061`50afbad0 00007ff9`9a868c4c     sourcekitdInProc!swift::Evaluator::getResultCached<swift::ResolveTypeWitnessesRequest,`swift::evaluateOrDefault<swift::ResolveTypeWitnessesRequest>'::`2'::<lambda_1>,0>+0x178
0f 00000061`50afbb80 00007ff9`9a865a74     sourcekitdInProc!swift::TypeChecker::checkConformancesInContext+0x299c
10 00000061`50afbe50 00007ff9`9a866a1f     sourcekitdInProc!swift::ConformanceChecker::checkActorIsolation+0x1004
11 00000061`50afc0f0 00007ff9`9ab389b0     sourcekitdInProc!swift::TypeChecker::checkConformancesInContext+0x76f
12 00000061`50afc950 00007ff9`9ab33eea     sourcekitdInProc!swift::TypeChecker::typeCheckDecl+0x65f0
13 00000061`50afcc00 00007ff9`9ab3241f     sourcekitdInProc!swift::TypeChecker::typeCheckDecl+0x1b2a
14 00000061`50afcec0 00007ff9`9a7e1d01     sourcekitdInProc!swift::TypeChecker::typeCheckDecl+0x5f
15 00000061`50afcf00 00007ff9`9a7ba0ca     sourcekitdInProc!swift::TypeCheckSourceFileRequest::evaluate+0x151
16 00000061`50afcfd0 00007ff9`9a7da3e3     sourcekitdInProc!swift::SimpleRequest<swift::TypeCheckSourceFileRequest,std::tuple<> __cdecl(swift::SourceFile * __ptr64),12>::evaluateRequest+0x1a
17 00000061`50afd000 00007ff9`9a7e5ed8     sourcekitdInProc!swift::Evaluator::getResultUncached<swift::TypeCheckSourceFileRequest,`swift::evaluateOrDefault<swift::TypeCheckSourceFileRequest>'::`2'::<lambda_1> >+0x1f3
18 00000061`50afd130 00007ff9`99565b4c     sourcekitdInProc!swift::performTypeChecking+0x168
19 00000061`50afd1b0 00007ff9`9956dde8     sourcekitdInProc!swift::evaluator::DependencyRecorder::beginRequest<swift::IDEInspectionFileRequest>+0xec
1a 00000061`50afd1e0 00007ff9`995728d5     sourcekitdInProc!swift::CompilerInstance::forEachFileToTypeCheck+0x128
1b 00000061`50afd220 00007ff9`96e6b159     sourcekitdInProc!swift::CompilerInstance::performSema+0x95
1c 00000061`50afd2c0 00007ff9`96bf328e     sourcekitdInProc!llvm::SmallVectorImpl<std::shared_ptr<SourceKit::SwiftASTConsumer> >::erase+0x1b69
1d 00000061`50aff720 00007ff9`96bf32be     sourcekitdInProc!SourceKit::WorkQueue::Impl::release+0x8e
1e 00000061`50aff750 00007ffa`8c3c9333     sourcekitdInProc!llvm::thread::ThreadProxy<std::tuple<void (__cdecl*)(void * __ptr64),void * __ptr64> >+0xe
1f 00000061`50aff780 00007ffa`38c7786e     ucrtbase!thread_start<unsigned int (__cdecl*)(void *),1>+0x93
20 00000061`50aff7b0 00007ffa`8cc2257d     vfbasics!AVrfpStandardThreadFunction+0x4e
21 00000061`50aff7f0 00007ffa`8ef0af28     KERNEL32!BaseThreadInitThunk+0x1d
22 00000061`50aff820 00000000`00000000     ntdll!RtlUserThreadStart+0x28

[compnerd]

This seems reasonable to me - just because some platforms allow I/O to files which are backing MMIO doesn't mean we should assume that all platforms do. I wonder if there is an explicit guarantee that MMIO backing is preserved under modification (feels like it would have to disable CoW for this to work?).

[tristanlabelle]

I'm excited for this fix, but I wonder if there's a better way to pipe the flag through

[z2oh]

cc @ahoppen: would appreciate any thoughts you have here!

[ahoppen]

I would prefer to not have a distinction between SourceKit and a compilation here. I imagine that you could run into a similar issue during compilation if the source file takes a considerable amount of time to compile. Could we instead check if the current platform is Windows and, if so, always pass true for IsVolatile?

[compnerd]

I don't think that's a good idea. This is going to increase memory pressure on windows, which already has a slower build rate due to the fork exec heavy nature of the swift compiler (clang does an in process cc1). Not being able to save on top of the file during compilation is fine (and expected behavior on windows). When the file is being opened by LSP, it doesn't get closed again, which prevents subsequent writes indefinitely.

[bnbarham]

Can we limit this to Windows only then? And possibly also rename EditorMode to... something 😅? OpenAsVolatile? Any files opened by the editor are already in-memory anyway, so it seems odd to open any other files as volatile generally.

When the file is being opened by LSP

Just to be clear, when LSP opens a file we create an in-memory buffer. If this fixes the issue then it's the secondary files in the same module that are being opened.

Also seems like there's some underlying bug here somewhere too, since I wouldn't expect

it doesn't get closed again

[compnerd]

Can we limit this to Windows only then?

I'm okay with scoping this to Windows. I don't remember if POSIX guarantees that the mmap I/O backing store is modifiable when mapped.

Also seems like there's some underlying bug here somewhere too, since I wouldn't expect

it doesn't get closed again

Hmm, with mmap'ed data, it needs to remain mapped right? Or is the data processed and then the buffer dropped?

[bnbarham]

Hmm, with mmap'ed data, it needs to remain mapped right? Or is the data processed and then the buffer dropped?

I would naively expect the latter, but it's quite possible we're holding the files open while the ASTContext is alive (and maybe we do actually need to). That shouldn't be indefinitely, could be a somewhat lengthy period though.

[compnerd]

I would naively expect the latter, but it's quite possible we're holding the files open while the ASTContext is alive (and maybe we do actually need to). That shouldn't be indefinitely, could be a somewhat lengthy period though.

I would expect it to be kept while the ASTContext is alive as the MemoryBuffer will vend StringRefs and those would become invalid should the backing buffer disappear. The problem is that during the duration that it kept alive, the file is unwritable, including for saving edits from the editor.

[z2oh]

I'm happy to scope this down to Windows only and to rename EditorMode. This probably warrants the addition of a new flag as well, instead of glomming onto DiagnosticsEditorMode. Thank you for the feedback, I will update the PR soon 👍

The buffer is loaded and owned by SourceManager, which is owned by a CompilerInstance. I'm looking at the lifetime of a CompilerInstance w.r.t sourcekitd; observationally, this object seems to be recreated when you switch open files/modules in the editor. However, once a file is opened in this way, it appears to stay open until the sourcekitd process is killed. I'll do some more investigation here.

That said, avoiding mmapping source files opened by sourcekitd on Windows is desirable regardless of the length of time they are held open, as even transient save failures are a frustrating experience.

[bnbarham]

@swift-ci please test

[compnerd]

I think that this is ready to merge, going to merge to improve the windows state.