Hi Jan, I don't think there's much point in running ReplayPreamble with an empty preamble, but this should already be a no-op as there can't be any includes inside the preamble region if size is 0.
I can't seem to reproduce a failure with the root causes you've provided. Even when ReplayPreamble::create doesn't take the early exit, for an empty preamble we should not have any includes, hence ReplayPreamble::replay would be a no-op. That's what I am getting while running the tests (you can check this by printing the MainFileIncludes in ReplayPreamble::create before early exit. Note that PatchesAdditionalIncludes builds two ASTs, the first one will have additional includes as it builds a full AST with a non-empty preamble, but the second AST should be built with an empty preamble and empty preamble-includes) It seems like something else is going on here, any chance you are inserting an implicit include inside your custom PP logic? If that's the case we should look for a fix in include extraction logic as it shouldn't pick up includes that are coming from implicit(more over non-main-file) sources. On Tue, Jun 2, 2020 at 8:09 AM Jan Korous via Phabricator < revi...@reviews.llvm.org> wrote: > jkorous added a comment. > > Hi @kadircet! > I am investigating a failure of `PatchesAdditionalIncludes` test that we > got internally. It's a failed assert in `ReplayPreamble::replay`. > Our clangd source code is for all practical purposes identical to upstream > version but not so with clang source. Specifically what we do is that in > `CompilerInstance::createPreprocessor` we always add one particular > callback. > This means that when in the test we are calling `buildPreamble` for > `TestTU TU` with empty buffer we never hit the early return in > `ReplayPreamble::attach()` (ParsedAST.cpp:124) like upstream version does > and proceed to create a new `ReplayPreamble` object with `PreambleBounds` > of `size() == 0` which leads to `ReplayPreamble::MainFileTokens` to be > empty and later we hit failed assert in `ReplayPreamble::replay` about > `assert(HashTok != MainFileTokens.end() && ...)`. > > Now, while I can just tweak either `ReplayPreamble::attach()` or remove > the PP callback in the test internally I am wondering whether you consider > empty preamble & PP with callbacks valid use-case of `ReplayPreamble` and > worth a fix. > > This is where we are creating the empty `MainFileTokens`: > > * frame #0: 0x0000000103337649 ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::ReplayPreamble(this=0x0000000114f45da0, > Includes=0x00007ffeefbfc678, Delegate=0x0000000114f3bd80, > SM=0x0000000114f3dd80, PP=0x0000000115850218, LangOpts=0x0000000114f38eb0, > PB=0x00007ffeefbfc658) + 169 at ParsedAST.cpp:142 > frame #1: 0x000000010333756d ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::ReplayPreamble(this=0x0000000114f45da0, > Includes=0x00007ffeefbfc678, Delegate=0x0000000114f3bd80, > SM=0x0000000114f3dd80, PP=0x0000000115850218, LangOpts=0x0000000114f38eb0, > PB=0x00007ffeefbfc658) + 77 at ParsedAST.cpp:139 > frame #2: 0x0000000103334fa7 ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::attach(Includes=0x00007ffeefbfc678, > Clang=0x0000000114f33ea0, PB=0x00007ffeefbfc658) + 183 at ParsedAST.cpp:126 > frame #3: 0x0000000103334189 ClangdTests` > clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", > Length = 20), Inputs=0x00007ffeefbfdf98, CI=nullptr, > CompilerInvocationDiags=ArrayRef<clang::clangd::Diag> @ 0x00007ffeefbfd830, > Preamble=std::__1::shared_ptr<const > clang::clangd::PreambleData>::element_type @ 0x0000000114f3e728 strong=2 > weak=1) + 3897 at ParsedAST.cpp:385 > frame #4: 0x00000001006f1032 ClangdTests` clang::clangd::(anonymous > namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x0000000114f04090) > + 1778 at ParsedASTTests.cpp:477 > > This is the failed assert: > > * frame #4: 0x0000000103337a0c ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::replay(this=0x0000000114f45da0) + 556 at > ParsedAST.cpp:182 > frame #5: 0x000000010333777c ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::FileChanged(this=0x0000000114f45da0, Loc=(ID = > 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2)) + 156 at > ParsedAST.cpp:166 > frame #6: 0x0000000101b7900a ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x0000000116204080, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 90 at > PPCallbacks.h:390 > frame #7: 0x0000000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x00000001162040c0, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #8: 0x0000000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x0000000116204100, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #9: 0x0000000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x0000000116204160, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #10: 0x0000000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x0000000116205480, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #11: 0x0000000101b9a590 ClangdTests` > clang::Preprocessor::HandleEndOfFile(this=0x0000000115850218, > Result=0x0000000116808210, isEndOfMacro=false) + 3904 at > PPLexerChange.cpp:469 > frame #12: 0x0000000101b38713 ClangdTests` > clang::Lexer::LexEndOfFile(this=0x0000000116206330, > Result=0x0000000116808210, CurPtr="") + 931 at Lexer.cpp:2833 > frame #13: 0x0000000101b39c44 ClangdTests` > clang::Lexer::LexTokenInternal(this=0x0000000116206330, > Result=0x0000000116808210, TokAtPhysicalStartOfLine=true) + 420 at > Lexer.cpp:3265 > frame #14: 0x0000000101b382f8 ClangdTests` > clang::Lexer::Lex(this=0x0000000116206330, Result=0x0000000116808210) + > 216 at Lexer.cpp:3216 > frame #15: 0x0000000101bd7396 ClangdTests` > clang::Preprocessor::Lex(this=0x0000000115850218, > Result=0x0000000116808210) + 118 at Preprocessor.cpp:900 > frame #16: 0x0000000101b75ef1 ClangdTests` > clang::Preprocessor::CachingLex(this=0x0000000115850218, > Result=0x0000000116808210) + 289 at PPCaching.cpp:63 > frame #17: 0x0000000101bd73d5 ClangdTests` > clang::Preprocessor::Lex(this=0x0000000115850218, > Result=0x0000000116808210) + 181 at Preprocessor.cpp:906 > frame #18: 0x0000000104e3c21c ClangdTests` > clang::Parser::TryConsumeToken(this=0x0000000116808200, Expected=semi) + > 204 at Parser.h:489 > frame #19: 0x0000000104e3bf1a ClangdTests` > clang::Parser::ExpectAndConsumeSemi(this=0x0000000116808200, DiagID=1526) > + 42 at Parser.cpp:162 > frame #20: 0x0000000104d5f0e0 ClangdTests` > clang::Parser::ParseDeclGroup(this=0x0000000116808200, > DS=0x00007ffeefbfb6e8, Context=FileContext, DeclEnd=0x0000000000000000, > FRI=0x0000000000000000) + 3168 at ParseDecl.cpp:2243 > frame #21: 0x0000000104e42c48 ClangdTests` > clang::Parser::ParseDeclOrFunctionDefInternal(this=0x0000000116808200, > attrs=0x00007ffeefbfbc80, DS=0x00007ffeefbfb6e8, AS=AS_none) + 1704 at > Parser.cpp:1109 > frame #22: 0x0000000104e42170 ClangdTests` > clang::Parser::ParseDeclarationOrFunctionDefinition(this=0x0000000116808200, > attrs=0x00007ffeefbfbc80, DS=0x0000000000000000, AS=AS_none) + 144 at > Parser.cpp:1125 > frame #23: 0x0000000104e411fa ClangdTests` > clang::Parser::ParseExternalDeclaration(this=0x0000000116808200, > attrs=0x00007ffeefbfbc80, DS=0x0000000000000000) + 3642 at Parser.cpp:945 > frame #24: 0x0000000104e3f254 ClangdTests` > clang::Parser::ParseTopLevelDecl(this=0x0000000116808200, > Result=0x00007ffeefbfbde8, IsFirstDecl=false) + 1828 at Parser.cpp:693 > frame #25: 0x0000000104d465e3 ClangdTests` > clang::ParseAST(S=0x000000011680c200, PrintStats=false, > SkipFunctionBodies=false) + 595 at ParseAST.cpp:158 > frame #26: 0x0000000101992662 ClangdTests` > clang::ASTFrontendAction::ExecuteAction(this=0x0000000114f349a0) + 322 at > FrontendAction.cpp:1066 > frame #27: 0x0000000101991b78 ClangdTests` > clang::FrontendAction::Execute(this=0x0000000114f349a0) + 136 at > FrontendAction.cpp:959 > frame #28: 0x00000001033343ab ClangdTests` > clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", > Length = 20), Inputs=0x00007ffeefbfdf98, CI=nullptr, > CompilerInvocationDiags=ArrayRef<clang::clangd::Diag> @ 0x00007ffeefbfd830, > Preamble=std::__1::shared_ptr<const > clang::clangd::PreambleData>::element_type @ 0x0000000114f3e728 strong=2 > weak=1) + 4443 at ParsedAST.cpp:415 > frame #29: 0x00000001006f1032 ClangdTests` clang::clangd::(anonymous > namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x0000000114f04090) > + 1778 at ParsedASTTests.cpp:477 > > > Repository: > rG LLVM Github Monorepo > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D77644/new/ > > https://reviews.llvm.org/D77644 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits