[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 2 02:08:46 PDT 2020 

On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya <kadircet at 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 <
> reviews at 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
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200602/6e17867d/attachment.html>

More information about the cfe-commits mailing list