[libclang] Avoid more stats than necessary for reparse.

[nik]

Summary

...by not stating the files in the preamble a second time.

[nik]

Hmm, looks like libclang/libclangTests/LibclangReparseTest.ReparseWithModule is flaky?

FAIL: Clang-Unit :: libclang/libclangTests/LibclangReparseTest.ReparseWithModule (10699 of 10701)
******************** TEST 'Clang-Unit :: libclang/libclangTests/LibclangReparseTest.ReparseWithModule' FAILED ********************
Note: Google Test filter = LibclangReparseTest.ReparseWithModule
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from LibclangReparseTest
[ RUN      ] LibclangReparseTest.ReparseWithModule
LIBCLANG FATAL ERROR: Unexpected end of file

********************
Testing Time: 61.30s
********************
Failing Tests (1):
    Clang-Unit :: libclang/libclangTests/LibclangReparseTest.ReparseWithModule

  Expected Passes    : 10621
  Expected Failures  : 19
  Unsupported Tests  : 60
  Unexpected Failures: 1
tools/clang/test/CMakeFiles/check-clang.dir/build.make:57: recipe for target 'tools/clang/test/CMakeFiles/check-clang' failed
make[3]: *** [tools/clang/test/CMakeFiles/check-clang] Error 1
CMakeFiles/Makefile2:33822: recipe for target 'tools/clang/test/CMakeFiles/check-clang.dir/all' failed
make[2]: *** [tools/clang/test/CMakeFiles/check-clang.dir/all] Error 2
CMakeFiles/Makefile2:33829: recipe for target 'tools/clang/test/CMakeFiles/check-clang.dir/rule' failed
make[1]: *** [tools/clang/test/CMakeFiles/check-clang.dir/rule] Error 2
Makefile:7932: recipe for target 'check-clang' failed
make: *** [check-clang] Error 2
zsh: exit 2     make check-clang

[nik]

Ping :)

[ilya-biryukov]

Sorry, didn't get a chance to look into it very thoroughly yet.

Are there any other callers to getMainBufferWithPrecompiledPreamble?
Maybe they cause LibclangReparseTest.ReparseWithModule to fail?

lib/Frontend/ASTUnit.cpp

1380

Are we relying on the caller to create new FileMgr before calling getMainBufferWithPrecompiledPreamble?
Otherwise we'll hit a cached entry and possibly miss an update to the file?

This deserves a comment if that's the case.

2051

Parse method used to recreate FileMgr from CompilerInvocation, because it was reset() at this point.
Won't something break because we're reusing the FileMgr now?
Maybe the code creating FileMgr in Parse should be deleted?

[nik]

In D33045#759436, @ilya-biryukov wrote:

Are there any other callers to getMainBufferWithPrecompiledPreamble?

Yes. Huch, right... don't know why I didn't adapted those. Done now.

Maybe they cause LibclangReparseTest.ReparseWithModule to fail?

So it looks like. Actually that one become flaky now and the other failing ones are:

Failing Tests (74):
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportAtomicExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportBinaryConditionalOperator
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportCXXNullPtrLiteralExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportCXXThisExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportCompoundLiteralExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportConditionalOperator
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportDesignatedInitExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportFloatinglLiteralExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportGNUNullExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportInitListExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportLabelDeclAndAddrLabelExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportParenListExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportPredefinedExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportStmtExpr
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportStringLiteral
    Clang-Unit :: AST/ASTTests/ImportExpr.ImportVAArgExpr
    Clang-Unit :: AST/ASTTests/ImportType.ImportAtomicType
    Clang-Unit :: AST/ASTTests/RecursiveASTVisitor.NoPostOrderTraversal
    Clang-Unit :: AST/ASTTests/RecursiveASTVisitor.PostOrderTraversal
    Clang-Unit :: ASTMatchers/ASTMatchersTests/AstMatcherPMacro.Works
    Clang-Unit :: ASTMatchers/ASTMatchersTests/AstPolymorphicMatcherPMacro.Works
    Clang-Unit :: ASTMatchers/ASTMatchersTests/EachOf.BehavesLikeAnyOfUnlessBothMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/EachOf.TriggersForEachMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/EqualsBoundNodeMatcher.FiltersMatchedCombinations
    Clang-Unit :: ASTMatchers/ASTMatchersTests/EqualsBoundNodeMatcher.UnlessDescendantsOfAncestorsMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/EqualsBoundNodeMatcher.UsingForEachDescendant
    Clang-Unit :: ASTMatchers/ASTMatchersTests/FindAll.BindsDescendantNodeOnMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/FindAll.BindsNodeAndDescendantNodesOnOneMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/FindAll.BindsNodeOnMatch
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEach.BindsMultipleNodes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEach.BindsOneNode
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEach.BindsRecursiveCombinations
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachArgumentWithParam.HandlesBoundNodesForNonMatches
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachArgumentWithParam.MatchesCXXMemberCallExpr
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachArgumentWithParam.MatchesCallExpr
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachArgumentWithParam.MatchesConstructExpr
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.BindsCombinations
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.BindsCorrectNodes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.BindsMultipleNodes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.BindsOneNode
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.BindsRecursiveCombinations
    Clang-Unit :: ASTMatchers/ASTMatchersTests/ForEachDescendant.NestedForEachDescendant
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Has.DoesNotDeleteBindings
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Has.MatchesChildrenOfTypes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/HasAncestor.BindsCombinationsWithHasDescendant
    Clang-Unit :: ASTMatchers/ASTMatchersTests/HasAncestor.BindsRecursiveCombinations
    Clang-Unit :: ASTMatchers/ASTMatchersTests/HasAncestor.MatchesClosestAncestor
    Clang-Unit :: ASTMatchers/ASTMatchersTests/HasDescendant.MatchesDescendantTypes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/HasDescendant.MatchesDescendantsOfTypes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/IsEqualTo.MatchesNodesByIdentity
    Clang-Unit :: ASTMatchers/ASTMatchersTests/LoopingMatchers.DoNotOverwritePreviousMatchResultOnFailure
    Clang-Unit :: ASTMatchers/ASTMatchersTests/MatchFinder.CanMatchDeclarationsRecursively
    Clang-Unit :: ASTMatchers/ASTMatchersTests/MatchFinder.CanMatchSingleNodesRecursively
    Clang-Unit :: ASTMatchers/ASTMatchersTests/MatchFinder.CanMatchStatementsRecursively
    Clang-Unit :: ASTMatchers/ASTMatchersTests/MatchFinder.InterceptsEndOfTranslationUnit
    Clang-Unit :: ASTMatchers/ASTMatchersTests/MatchFinder.InterceptsStartOfTranslationUnit
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.BindMatchedNodes
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.BindTheSameNameInAlternatives
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.BindsIDForMemoizedResults
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.ForEachOverriden
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.NestedOverloadedOperatorCalls
    Clang-Unit :: ASTMatchers/ASTMatchersTests/Matcher.matchOverEntireASTContext
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNS.BindsNestedNameSpecifierLocs
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNS.BindsNestedNameSpecifiers
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNS.DescendantsOfNestedNameSpecifiers
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNS.NestedNameSpecifiersAsDescendants
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNSLoc.DescendantsOfNestedNameSpecifierLocs
    Clang-Unit :: ASTMatchers/ASTMatchersTests/NNSLoc.NestedNameSpecifierLocsAsDescendants
    Clang-Unit :: ASTMatchers/ASTMatchersTests/SwitchCase.MatchesEachCase
    Clang-Unit :: Analysis/ClangAnalysisTests/CloneDetector.FilterFunctionsByName
    Clang-Unit :: Tooling/ToolingTests/ClangToolTest.BuildASTs
    Clang-Unit :: Tooling/ToolingTests/ClangToolTest.InjectDiagnosticConsumerInBuildASTs
    Clang-Unit :: Tooling/ToolingTests/buildASTFromCode.FindsClassDecl
    Clang-Unit :: libclang/libclangTests/LibclangReparseTest.ReparseWithModule

  Expected Passes    : 10572
  Expected Failures  : 19
  Unsupported Tests  : 60
  Unexpected Failures: 74
FAILED: tools/clang/test/CMakeFiles/check-clang 
cd /home/nik/dev/llvm/trunk/builds/gcc63_rtti/tools/clang/test && /usr/bin/python2.7 /home/nik/dev/llvm/trunk/source/utils/lit/lit.py -sv --param clang_site_config=/home/nik/dev/llvm/trunk/builds/gcc63_rtti/tools/clang/test/lit.site.cfg /home/nik/dev/llvm/trunk/builds/gcc63_rtti/tools/clang/test
ninja: build stopped: subcommand failed.

Hmm, I don't understand yet the connection to this change. Most of them fail because of the added recreateFileManager() to ASTUnit::LoadFromCompilerInvocation().

lib/Frontend/ASTUnit.cpp

1380

Yes, the caller is now supposed to create the FileMgr. Added the comment.

2051

Parse method used to recreate FileMgr from CompilerInvocation, because it was reset() at this point.

It still does so for the initial parse (clang_parseTranslationUnit).

Won't something break because we're reusing the FileMgr now?

Hmm, now that I've adapted the other callers I get failing tests :/

Maybe the code creating FileMgr in Parse should be deleted?

It's still needed for the initial parse. We could move the initial creation to another point of course, but I do not see the advantage of this.

[ilya-biryukov]

I've added a few more comments, but I'm not really helping much here. Someone more experienced with ASTUnit is certainly needed.
Also, please be sure to check clangd(from clang-tools-extra) before submitting, it's also a client of ASTUnit that might've breaked.

include/clang/Frontend/ASTUnit.h

521

It would be really unfortunate to have that as a part of public API.

lib/Frontend/ASTUnit.cpp

2051

My point was that the intricacies of FileManager management in ASTUnit are already complicated enough. So it would be really nice if we could get rid of another point where FileManager is getting created.

I'm not really familiar with those tests, so I don't have a clue why they might be failing.

tools/libclang/CIndexCodeCompletion.cpp

683

It feels like there should be way to do it without changing public API.
Do we really need this call?

[nik]

I do not have time to work on this right now, Abandoning.

include/clang/Frontend/ASTUnit.h

public:
   const LangOptions &getLangOpts() const {
     assert(LangOpts && " ASTUnit does not have language options");
     return *LangOpts;
   }
   
   const FileManager &getFileManager() const { return *FileMgr; }
         FileManager &getFileManager()       { return *FileMgr; }
 
+  void recreateFileManager();
+
   const FileSystemOptions &getFileSystemOpts() const { return FileSystemOpts; }
 
   IntrusiveRefCntPtr<ASTReader> getASTReader() const;
 
   StringRef getOriginalSourceFileName() {
     return OriginalSourceFile;
   }
 

lib/Frontend/ASTUnit.cpp

ASTUnit::~ASTUnit() {
   if (getenv("LIBCLANG_OBJTRACKING"))
     fprintf(stderr, "--- %u translation units\n", --ActiveASTUnitObjects);
 }
 
 void ASTUnit::setPreprocessor(std::shared_ptr<Preprocessor> PP) {
   this->PP = std::move(PP);
 }
 
+void ASTUnit::recreateFileManager() {
+    FileMgr = new FileManager(FileMgr->getFileSystemOpts());
+}
+
 /// \brief Determine the set of code-completion contexts in which this 
 /// declaration should be shown.
 static unsigned getDeclShowContexts(const NamedDecl *ND,
                                     const LangOptions &LangOpts,
                                     bool &IsNestedNameSpecifier) {
   IsNestedNameSpecifier = false;
   
   if (isa<UsingShadowDecl>(ND))

 ///
 /// This routine will compute the preamble of the main source file. If a
 /// non-trivial preamble is found, it will precompile that preamble into a 
 /// precompiled header so that the precompiled preamble can be used to reduce
 /// reparsing time. If a precompiled preamble has already been constructed,
 /// this routine will determine if it is still valid and, if so, avoid 
 /// rebuilding the precompiled preamble.
 ///
+/// The caller is required to recreate the FileMgr before calling this routine
+/// to ensure a clean stat cache and on the other hand that the stats done here
+/// can be reused for e.g. Reparse().
+///
 /// \param AllowRebuild When true (the default), this routine is
 /// allowed to rebuild the precompiled preamble if it is found to be
 /// out-of-date.
 ///
 /// \param MaxLines When non-zero, the maximum number of lines that
 /// can occur within the preamble.
 ///
 /// \returns If the precompiled preamble can be used, returns a newly-allocated
if (Preamble.size() == NewPreamble.Size &&
           
       // First, make a record of those files that have been overridden via
       // remapping or unsaved_files.
       std::map<llvm::sys::fs::UniqueID, PreambleFileHash> OverriddenFiles;
       for (const auto &R : PreprocessorOpts.RemappedFiles) {
         if (AnyFileChanged)
           break;
 
-        vfs::Status Status;
-        if (FileMgr->getNoncachedStatValue(R.second, Status)) {
+        const FileEntry *fileEntry = FileMgr->getFile(R.second);
+        if (!fileEntry) {
           // If we can't stat the file we're remapping to, assume that something
           // horrible happened.
           AnyFileChanged = true;
           break;
         }
 
-        OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
-            Status.getSize(),
-            llvm::sys::toTimeT(Status.getLastModificationTime()));
+        OverriddenFiles[fileEntry->getUniqueID()] = PreambleFileHash::createForFile(
+            fileEntry->getSize(),
+            fileEntry->getModificationTime());
       }
 
       for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
         if (AnyFileChanged)
           break;
 
-        vfs::Status Status;
-        if (FileMgr->getNoncachedStatValue(RB.first, Status)) {
+        const FileEntry *fileEntry = FileMgr->getFile(RB.first);
+        if (!fileEntry) {
           AnyFileChanged = true;
           break;
         }
 
-        OverriddenFiles[Status.getUniqueID()] =
+        OverriddenFiles[fileEntry->getUniqueID()] =
             PreambleFileHash::createForMemoryBuffer(RB.second);
       }
        
       // Check whether anything has changed.
       for (llvm::StringMap<PreambleFileHash>::iterator
              F = FilesInPreamble.begin(), FEnd = FilesInPreamble.end();
            !AnyFileChanged && F != FEnd; 
            ++F) {
-        vfs::Status Status;
-        if (FileMgr->getNoncachedStatValue(F->first(), Status)) {
+        const FileEntry *fileEntry = FileMgr->getFile(F->first());
+        if (!fileEntry) {
           // If we can't stat the file, assume that something horrible happened.
           AnyFileChanged = true;
           break;
         }
 
         std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden
-          = OverriddenFiles.find(Status.getUniqueID());
+          = OverriddenFiles.find(fileEntry->getUniqueID());
         if (Overridden != OverriddenFiles.end()) {
           // This file was remapped; check whether the newly-mapped file 
           // matches up with the previous mapping.
           if (Overridden->second != F->second)
             AnyFileChanged = true;
           continue;
         }
-        
         // The file was not remapped; check whether it has changed on disk.
-        if (Status.getSize() != uint64_t(F->second.Size) ||
-            llvm::sys::toTimeT(Status.getLastModificationTime()) !=
-                F->second.ModTime)
+        if (fileEntry->getSize() != F->second.Size ||
+            fileEntry->getModificationTime() != F->second.ModTime)
           AnyFileChanged = true;
       }
           
       if (!AnyFileChanged) {
         // Okay! We can re-use the precompiled preamble.
 
         // Set the state of the diagnostic object to mimic its state
         // after parsing the preamble.
if (!Invocation)
     return true;
   
   // We'll manage file buffers ourselves.
   Invocation->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   Invocation->getFrontendOpts().DisableFree = false;
   getDiagnostics().Reset();
   ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
 
+  recreateFileManager();
+
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
     PreambleRebuildCounter = PrecompilePreambleAfterNParses;
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation);
     getDiagnostics().Reset();
     ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
   }
for (const auto &RB : PPOpts.RemappedFileBuffers)
     delete RB.second;
 
   Invocation->getPreprocessorOpts().clearRemappedFiles();
   for (const auto &RemappedFile : RemappedFiles) {
     Invocation->getPreprocessorOpts().addRemappedFile(RemappedFile.first,
                                                       RemappedFile.second);
   }
 
+  recreateFileManager();
+
   // If we have a preamble file lying around, or if we might try to
   // build a precompiled preamble, do so now.
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (!getPreambleFile(this).empty() || PreambleRebuildCounter > 0)
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation);
 
   // Clear out the diagnostics state.
-  FileMgr.reset();
   getDiagnostics().Reset();
   ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
   if (OverrideMainBuffer)
     getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
   // Parse the sources
   bool Result =
       Parse(std::move(PCHContainerOps), std::move(OverrideMainBuffer));

tools/libclang/CIndexCodeCompletion.cpp

for (auto &UF : unsaved_files) {
     RemappedFiles.push_back(std::make_pair(UF.Filename, MB.release()));
   }
 
   if (EnableLogging) {
     // FIXME: Add logging.
   }
 
   // Parse the resulting source file to find code-completion results.
+  AST->recreateFileManager();
   AllocatedCXCodeCompleteResults *Results = new AllocatedCXCodeCompleteResults(
       &AST->getFileManager());
   Results->Results = nullptr;
   Results->NumResults = 0;
   
   // Create a code-completion consumer to capture the results.
   CodeCompleteOptions Opts;
   Opts.IncludeBriefComments = IncludeBriefComments;