[Frontend] Honor UserFilesAreVolatile flag getting file buffer in ASTUnit

[yvvan]

Summary

Do not memory map the main file if the flag UserFilesAreVolatile is set to true in ASTUnit when calling FileSystem::getBufferForFile

[yvvan]

The precise path which leads here is yet unclear for me (it requires much more time to research) but changing this specific default value to true fixes main file from being memory mapped and therefore being blocked (on Windows)
I assume it comes from FileManager::getBufferForFile(StringRef Filename) but not sure about prior events causing it.

Usually the wrong behavior can be encountered via reparsing the file.

[ilya-biryukov]

It's a very subtle change and it isn't clear if that's the right thing to do without understanding the problem that we're trying to solve.
Could you please elaborate on the problem?

accidental memory mapping for main file which sometimes happens through FileSystem::getBufferForFile

Why is memory mapping considered accidental and what problems does it cause?

[yvvan]

Memory mapping locks the file on Windows. That means that in the addressed case it locks main file which we are editing. After that happens many tools struggle to do something with this file, for example git.
It's the quickest solution, probably not the best. Better one would introduce isVolatile parameter in FileSystem::getBufferForFile which should be set to true somewhere (I didn't have time to search for the specific location yet)

[yvvan]

This is the proper fix. When we get a buffer for main file we should use the UserFilesAreVolatile flag to specify if memory mapping needs to occur or not.

[ilya-biryukov]

LGTM. Is it plausible to add a unit-test for this?

[yvvan]

In D47460#1118694, @ilya-biryukov wrote:

Is it plausible to add a unit-test for this?

i think I can add a unit-test for it since we have the 'getBufferKind' method in MemoryBuffer.

[ilya-biryukov]

In D47460#1118782, @yvvan wrote:

i think I can add a unit-test for it since we have the 'getBufferKind' method in MemoryBuffer.

That sounds good. Having a regression test that fails with descriptive messages in case anyone changes this would certainly be useful.
If that's not a lot of work, of course.

[yvvan]

I could not properly test getMainBufferWithPrecompiledPreamble because it's private and requires some extra context to be called.

But there's another getBufferForFile call in ASTUnit which has similar symptoms and might cause wrong mmap behavior. I've also appended it with the same parameter 'UserFilesAreVolatile' and added unit-test for it.

[yvvan]

fix indentation in unit-test

[orgads]

unittests/Frontend/ASTUnitTest.cpp

21

Macros are evil. Maybe use TEST_F, and make these members of the base class?

[yvvan]

Use fixture in unit-test

include/clang/Basic/FileManager.h

const FileEntry *getVirtualFile(StringRef Filename, off_t Size,
                                   time_t ModificationTime);
 
   /// 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,
                    bool ShouldCloseOpenFile = true);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFile(StringRef Filename);
+  getBufferForFile(StringRef Filename, bool isVolatile = false);
 
   /// Get the 'stat' information for the given \p Path.
   ///
   /// If the path is relative, it will be resolved against the WorkingDir of the
   /// FileManager's FileSystemOptions.
   ///
   /// \returns false on success, true on error.
   bool getNoncachedStatValue(StringRef Path,

lib/Basic/FileManager.cpp

FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
 
   SmallString<128> FilePath(Entry->getName());
   FixupRelativePath(FilePath);
   return FS->getBufferForFile(FilePath, FileSize,
                               /*RequiresNullTerminator=*/true, isVolatile);
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-FileManager::getBufferForFile(StringRef Filename) {
+FileManager::getBufferForFile(StringRef Filename, bool isVolatile) {
   if (FileSystemOpts.WorkingDir.empty())
-    return FS->getBufferForFile(Filename);
+    return FS->getBufferForFile(Filename, -1, true, isVolatile);
 
   SmallString<128> FilePath(Filename);
   FixupRelativePath(FilePath);
-  return FS->getBufferForFile(FilePath.c_str());
+  return FS->getBufferForFile(FilePath.c_str(), -1, true, isVolatile);
 }
 
 /// getStatValue - Get the 'stat' information for the specified path,
 /// using the cache to accelerate it if possible.  This returns true
 /// if the path points to a virtual file or does not exist, or returns
 /// false if it's an existent real file.  If FileDescriptor is NULL,
 /// do directory look-up instead of file look-up.
 bool FileManager::getStatValue(StringRef Path, FileData &Data, bool isFile,

lib/Frontend/ASTUnit.cpp

static bool moveOnNoError(llvm::ErrorOr<T> Val, T &Output) {
   return true;
 }
 
 /// Get a source buffer for \p MainFilePath, handling all file-to-file
 /// and file-to-buffer remappings inside \p Invocation.
 static std::unique_ptr<llvm::MemoryBuffer>
 getBufferForFileHandlingRemapping(const CompilerInvocation &Invocation,
                                   vfs::FileSystem *VFS,
-                                  StringRef FilePath) {
+                                  StringRef FilePath,
+                                  bool isVolatile) {
   const auto &PreprocessorOpts = Invocation.getPreprocessorOpts();
 
   // Try to determine if the main file has been remapped, either from the
   // command line (to another file) or directly through the compiler
   // invocation (to a memory buffer).
   llvm::MemoryBuffer *Buffer = nullptr;
   std::unique_ptr<llvm::MemoryBuffer> BufferOwner;
   auto FileStatus = VFS->status(FilePath);
   if (FileStatus) {
     llvm::sys::fs::UniqueID MainFileID = FileStatus->getUniqueID();
 
     // Check whether there is a file-file remapping of the main file
     for (const auto &RF : PreprocessorOpts.RemappedFiles) {
       std::string MPath(RF.first);
       auto MPathStatus = VFS->status(MPath);
       if (MPathStatus) {
         llvm::sys::fs::UniqueID MID = MPathStatus->getUniqueID();
         if (MainFileID == MID) {
           // We found a remapping. Try to load the resulting, remapped source.
-          BufferOwner = valueOrNull(VFS->getBufferForFile(RF.second));
+          BufferOwner = valueOrNull(VFS->getBufferForFile(RF.second, -1, true, isVolatile));
           if (!BufferOwner)
             return nullptr;
         }
       }
     }
 
     // Check whether there is a file-buffer remapping. It supercedes the
     // file-file remapping.
     for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
       std::string MPath(RB.first);
       auto MPathStatus = VFS->status(MPath);
       if (MPathStatus) {
         llvm::sys::fs::UniqueID MID = MPathStatus->getUniqueID();
         if (MainFileID == MID) {
           // We found a remapping.
           BufferOwner.reset();
           Buffer = const_cast<llvm::MemoryBuffer *>(RB.second);
         }
       }
     }
   }
 
   // If the main source file was not remapped, load it now.
   if (!Buffer && !BufferOwner) {
-    BufferOwner = valueOrNull(VFS->getBufferForFile(FilePath));
+    BufferOwner = valueOrNull(VFS->getBufferForFile(FilePath, -1, true, isVolatile));
     if (!BufferOwner)
       return nullptr;
   }
 
   if (BufferOwner)
     return BufferOwner;
   if (!Buffer)
     return nullptr;
ASTDeserializationListener *ASTUnit::getDeserializationListener() {
   if (WriterData)
     return &WriterData->Writer;
   return nullptr;
 }
 
 std::unique_ptr<llvm::MemoryBuffer>
 ASTUnit::getBufferForFile(StringRef Filename, std::string *ErrorStr) {
   assert(FileMgr);
-  auto Buffer = FileMgr->getBufferForFile(Filename);
+  auto Buffer = FileMgr->getBufferForFile(Filename, UserFilesAreVolatile);
   if (Buffer)
     return std::move(*Buffer);
   if (ErrorStr)
     *ErrorStr = Buffer.getError().message();
   return nullptr;
 }
 
 /// Configure the diagnostics object for use with ASTUnit.
ASTUnit::getMainBufferWithPrecompiledPreamble(
     std::shared_ptr<PCHContainerOperations> PCHContainerOps,
     CompilerInvocation &PreambleInvocationIn,
     IntrusiveRefCntPtr<vfs::FileSystem> VFS, bool AllowRebuild,
     unsigned MaxLines) {
   auto MainFilePath =
       PreambleInvocationIn.getFrontendOpts().Inputs[0].getFile();
   std::unique_ptr<llvm::MemoryBuffer> MainFileBuffer =
       getBufferForFileHandlingRemapping(PreambleInvocationIn, VFS.get(),
-                                        MainFilePath);
+                                        MainFilePath, UserFilesAreVolatile);
   if (!MainFileBuffer)
     return nullptr;
 
   PreambleBounds Bounds =
       ComputePreambleBounds(*PreambleInvocationIn.getLangOpts(),
                             MainFileBuffer.get(), MaxLines);
   if (!Bounds.Size)
     return nullptr;

unittests/Frontend/ASTUnitTest.cpp

 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "gtest/gtest.h"
 
+#define CREATE_AST_UNIT(isVolatile)                                            \
+  int FD;                                                                      \
+  llvm::SmallString<256> InputFileName;                                        \
+  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("ast-unit", "cpp", FD,       \
+                                                  InputFileName));             \
+  ToolOutputFile input_file(InputFileName, FD);                                \
+  input_file.os() << "";                                                       \
+                                                                               \
+  const char *Args[] = {"clang", "-xc++", InputFileName.c_str()};              \
+                                                                               \
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =                                \
+      CompilerInstance::createDiagnostics(new DiagnosticOptions());            \
+                                                                               \
+  std::shared_ptr<CompilerInvocation> CInvok =                                 \
+      createInvocationFromCommandLine(Args, Diags);                            \
+                                                                               \
+  if (!CInvok)                                                                 \
+    FAIL() << "could not create compiler invocation";                          \
+                                                                               \
+  FileManager *FileMgr =                                                       \
+      new FileManager(FileSystemOptions(), vfs::getRealFileSystem());          \
+  auto PCHContainerOps = std::make_shared<PCHContainerOperations>();           \
+                                                                               \
+  std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromCompilerInvocation(          \
+      CInvok, PCHContainerOps, Diags, FileMgr, false, false, 0, TU_Complete,   \
+      false, false, isVolatile);
+
 using namespace llvm;
 using namespace clang;
 
 namespace {
 
 TEST(ASTUnit, SaveLoadPreservesLangOptionsInPrintingPolicy) {
   // Check that the printing policy is restored with the correct language
   // options when loading an ASTUnit from a file.  To this end, an ASTUnit
   // for a C++ translation unit is set up and written to a temporary file.
 
   // By default `UseVoidForZeroParams` is true for non-C++ language options,
   // thus we can check this field after loading the ASTUnit to deduce whether
   // the correct (C++) language options were used when setting up the printing
   // policy.
 
   {
     PrintingPolicy PolicyWithDefaultLangOpt(LangOptions{});
     EXPECT_TRUE(PolicyWithDefaultLangOpt.UseVoidForZeroParams);
   }
 
-  int FD;
-  llvm::SmallString<256> InputFileName;
-  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("ast-unit", "cpp", FD, InputFileName));
-  ToolOutputFile input_file(InputFileName, FD);
-  input_file.os() << "";
-
-  const char* Args[] = {"clang", "-xc++", InputFileName.c_str()};
-
-  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
-      CompilerInstance::createDiagnostics(new DiagnosticOptions());
-
-  std::shared_ptr<CompilerInvocation> CInvok =
-      createInvocationFromCommandLine(Args, Diags);
-
-  if (!CInvok)
-    FAIL() << "could not create compiler invocation";
-
-  FileManager *FileMgr =
-      new FileManager(FileSystemOptions(), vfs::getRealFileSystem());
-  auto PCHContainerOps = std::make_shared<PCHContainerOperations>();
-
-  std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromCompilerInvocation(
-      CInvok, PCHContainerOps, Diags, FileMgr);
+  CREATE_AST_UNIT(false);
 
   if (!AST)
     FAIL() << "failed to create ASTUnit";
 
   EXPECT_FALSE(AST->getASTContext().getPrintingPolicy().UseVoidForZeroParams);
 
   llvm::SmallString<256> ASTFileName;
   ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("ast-unit", "ast", FD, ASTFileName));
   ToolOutputFile ast_file(ASTFileName, FD);
   AST->Save(ASTFileName.str());
 
   EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
 
   std::unique_ptr<ASTUnit> AU = ASTUnit::LoadFromASTFile(
       ASTFileName.str(), PCHContainerOps->getRawReader(), ASTUnit::LoadEverything, Diags,
       FileSystemOptions(), /*UseDebugInfo=*/false);
 
   if (!AU)
     FAIL() << "failed to load ASTUnit";
 
   EXPECT_FALSE(AU->getASTContext().getPrintingPolicy().UseVoidForZeroParams);
 }
 
+TEST(ASTUnit, GetBufferForFileMemoryMapping) {
+  CREATE_AST_UNIT(true);
+
+  if (!AST)
+    FAIL() << "failed to create ASTUnit";
+
+  std::unique_ptr<llvm::MemoryBuffer> memoryBuffer =
+      AST->getBufferForFile(InputFileName);
+
+  EXPECT_NE(memoryBuffer->getBufferKind(),
+            llvm::MemoryBuffer::MemoryBuffer_MMap);
+}
+
 } // anonymous namespace