managed to reproduce the issue. sent out https://reviews.llvm.org/D80988 for a fix.
On Tue, Jun 2, 2020 at 12:08 PM Sam McCall <sammcc...@google.com> wrote: > On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya <kadir...@google.com> > wrote: > >> 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) >> > Yeah, this sounds suspicious to me. > In that loop just before the failed assert, can you dump the details of > `Inc`? > Specifically `Written`, `Resolved`, and HashOffset, and ideally work out > what buffer HashOffset really indexes into. > > >> >> 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
