This is an archive of the discontinued LLVM Phabricator instance.

[libclang] Avoid more stats than necessary for reparse.
AbandonedPublic

Authored by nik on May 10 2017, 8:36 AM.

Details

Summary

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

Diff Detail

Event Timeline

nik created this revision.May 10 2017, 8:36 AM
nik added reviewers: cfe-commits, bkramer, arphaman.EditedMay 10 2017, 8:37 AM

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 added a reviewer: akyrtzi.May 11 2017, 8:26 AM
bkramer edited reviewers, added: ilya-biryukov; removed: cfe-commits.May 19 2017, 7:07 AM
bkramer added a subscriber: cfe-commits.
ilya-biryukov edited edge metadata.May 19 2017, 7:32 AM

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 added a comment.May 22 2017, 4:35 AM

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.

nik updated this revision to Diff 99740.May 22 2017, 4:36 AM

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 abandoned this revision.Jul 11 2017, 1:20 AM

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