[PATCH] D33045: [libclang] Avoid more stats than necessary for reparse.

Mon May 22 04:35:31 PDT 2017

nik added a comment.

In https://reviews.llvm.org/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().

================
Comment at: lib/Frontend/ASTUnit.cpp:1395
+        const FileEntry *fileEntry = FileMgr->getFile(R.second);
+        if (!fileEntry) {
           // If we can't stat the file we're remapping to, assume that something
----------------
ilya-biryukov wrote:
> 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.
Yes, the caller is now supposed to create the FileMgr. Added the comment.

================
Comment at: lib/Frontend/ASTUnit.cpp:2074
   // Clear out the diagnostics state.
-  FileMgr.reset();
   getDiagnostics().Reset();
----------------
ilya-biryukov wrote:
> 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?
> 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.

https://reviews.llvm.org/D33045