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

[Mikael Holmén via cfe-commits]

On Thu, 2020-06-04 at 13:06 +0300, Kadir Çetinkaya wrote:
> Hi Mikael,
> 
> sent out 4f4a8ae72e95f2c7fa5e4ca56dd6b1a83a304680, please let me know
> if it helps!

Hi,

Yes, now it's silent. 

Thank you!
/Mikael

> 
> On Thu, Jun 4, 2020 at 12:40 PM Mikael Holmén via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > uabelho added inline comments.
> > 
> > 
> > 
> > Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1034
> >const PreambleData 
> > -  const PreamblePatch 
> > +  llvm::Optional Patch;
> >llvm::StringRef Contents;
> > 
> > Hi!
> > 
> > When compiling with gcc 7.4 I see a warning that I think originates
> > from this line:
> > 
> > ```
> > [4032/4668] Building CXX object
> > tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeC
> > omplete.cpp.o
> > In file included from
> > /data/repo/master/llvm/include/llvm/ADT/STLExtras.h:19:0,
> >  from
> > /data/repo/master/llvm/include/llvm/ADT/StringRef.h:12,
> >  from /data/repo/master/clang-tools-
> > extra/clangd/URI.h:12,
> >  from /data/repo/master/clang-tools-
> > extra/clangd/Protocol.h:26,
> >  from /data/repo/master/clang-tools-
> > extra/clangd/Headers.h:12,
> >  from /data/repo/master/clang-tools-
> > extra/clangd/CodeComplete.h:18,
> >  from /data/repo/master/clang-tools-
> > extra/clangd/CodeComplete.cpp:20:
> > /data/repo/master/llvm/include/llvm/ADT/Optional.h: In
> > instantiation of 'void llvm::optional_detail::OptionalStorage >  >::emplace(Args&& ...) [with Args = {const
> > clang::clangd::PreamblePatch}; T = const
> > clang::clangd::PreamblePatch; bool  = false]':
> > /data/repo/master/llvm/include/llvm/ADT/Optional.h:55:14: 
> >  required from 'llvm::optional_detail::OptionalStorage > 
> > >::OptionalStorage(llvm::optional_detail::OptionalStorage >  >&&) [with T = const clang::clangd::PreamblePatch; bool
> >  = false]'
> > /data/repo/master/llvm/include/llvm/ADT/Optional.h:228:3: 
> >  required from here
> > /data/repo/master/llvm/include/llvm/ADT/Optional.h:89:12: warning:
> > cast from type 'const clang::clangd::PreamblePatch*' to type
> > 'void*' casts away qualifiers [-Wcast-qual]
> >  ::new ((void *)std::addressof(value))
> > T(std::forward(args)...);
> > ^
> > ```
> > 
> > 
> > 
> > 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

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

[Kadir Çetinkaya via cfe-commits]

Hi Mikael,

sent out 4f4a8ae72e95f2c7fa5e4ca56dd6b1a83a304680, please let me know if it
helps!

On Thu, Jun 4, 2020 at 12:40 PM Mikael Holmén via Phabricator <
revi...@reviews.llvm.org> wrote:

> uabelho added inline comments.
>
>
> 
> Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1034
>const PreambleData 
> -  const PreamblePatch 
> +  llvm::Optional Patch;
>llvm::StringRef Contents;
> 
> Hi!
>
> When compiling with gcc 7.4 I see a warning that I think originates from
> this line:
>
> ```
> [4032/4668] Building CXX object
> tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o
> In file included from
> /data/repo/master/llvm/include/llvm/ADT/STLExtras.h:19:0,
>  from
> /data/repo/master/llvm/include/llvm/ADT/StringRef.h:12,
>  from /data/repo/master/clang-tools-extra/clangd/URI.h:12,
>  from
> /data/repo/master/clang-tools-extra/clangd/Protocol.h:26,
>  from
> /data/repo/master/clang-tools-extra/clangd/Headers.h:12,
>  from
> /data/repo/master/clang-tools-extra/clangd/CodeComplete.h:18,
>  from
> /data/repo/master/clang-tools-extra/clangd/CodeComplete.cpp:20:
> /data/repo/master/llvm/include/llvm/ADT/Optional.h: In instantiation of
> 'void llvm::optional_detail::OptionalStorage
> >::emplace(Args&& ...) [with Args = {const clang::clangd::PreamblePatch}; T
> = const clang::clangd::PreamblePatch; bool  = false]':
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:55:14:   required from
> 'llvm::optional_detail::OptionalStorage
> >::OptionalStorage(llvm::optional_detail::OptionalStorage
> >&&) [with T = const clang::clangd::PreamblePatch; bool  =
> false]'
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:228:3:   required from
> here
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:89:12: warning: cast
> from type 'const clang::clangd::PreamblePatch*' to type 'void*' casts away
> qualifiers [-Wcast-qual]
>  ::new ((void *)std::addressof(value)) T(std::forward(args)...);
> ^
> ```
>
>
>
> 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

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

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added inline comments.



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1034
   const PreambleData 
-  const PreamblePatch 
+  llvm::Optional Patch;
   llvm::StringRef Contents;

Hi!

When compiling with gcc 7.4 I see a warning that I think originates from this 
line:

```
[4032/4668] Building CXX object 
tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o
In file included from /data/repo/master/llvm/include/llvm/ADT/STLExtras.h:19:0,
 from /data/repo/master/llvm/include/llvm/ADT/StringRef.h:12,
 from /data/repo/master/clang-tools-extra/clangd/URI.h:12,
 from /data/repo/master/clang-tools-extra/clangd/Protocol.h:26,
 from /data/repo/master/clang-tools-extra/clangd/Headers.h:12,
 from 
/data/repo/master/clang-tools-extra/clangd/CodeComplete.h:18,
 from 
/data/repo/master/clang-tools-extra/clangd/CodeComplete.cpp:20:
/data/repo/master/llvm/include/llvm/ADT/Optional.h: In instantiation of 'void 
llvm::optional_detail::OptionalStorage >::emplace(Args&& ...) 
[with Args = {const clang::clangd::PreamblePatch}; T = const 
clang::clangd::PreamblePatch; bool  = false]':
/data/repo/master/llvm/include/llvm/ADT/Optional.h:55:14:   required from 
'llvm::optional_detail::OptionalStorage 
>::OptionalStorage(llvm::optional_detail::OptionalStorage >&&) 
[with T = const clang::clangd::PreamblePatch; bool  = false]'
/data/repo/master/llvm/include/llvm/ADT/Optional.h:228:3:   required from here
/data/repo/master/llvm/include/llvm/ADT/Optional.h:89:12: warning: cast from 
type 'const clang::clangd::PreamblePatch*' to type 'void*' casts away 
qualifiers [-Wcast-qual]
 ::new ((void *)std::addressof(value)) T(std::forward(args)...);
^
```



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

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

[Kadir Çetinkaya via cfe-commits]

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  wrote:

> On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya 
> 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: 0x000103337649 ClangdTests` clang::clangd::(anonymous
>>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
>>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
>>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
>>> PB=0x7ffeefbfc658)  + 169 at ParsedAST.cpp:142
>>> frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous
>>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
>>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
>>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
>>> PB=0x7ffeefbfc658)  + 77 at ParsedAST.cpp:139
>>> frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous
>>> namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678,
>>> Clang=0x000114f33ea0, PB=0x7ffeefbfc658)  + 183 at ParsedAST.cpp:126
>>> frame #3: 0x000103334189 ClangdTests`
>>> clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp",
>>> Length = 20), Inputs=0x7ffeefbfdf98, CI=nullptr,
>>> CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830,
>>> Preamble=std::__1::shared_ptr>> clang::clangd::PreambleData>::element_type @ 0x000114f3e728 strong=2
>>> weak=1)  + 3897 at ParsedAST.cpp:385
>>> frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous
>>> namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090)
>>> + 1778 at ParsedASTTests.cpp:477
>>>
>>> This is the failed assert:
>>>
>>>   * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous
>>> namespace)::ReplayPreamble::replay(this=0x000114f45da0)  + 556 at
>>> ParsedAST.cpp:182
>>> frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous
>>> namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID =
>>> 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2))  + 156 at
>>> ParsedAST.cpp:166
>>> frame #6: 0x000101b7900a 

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

[Sam McCall via cfe-commits]

On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya  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: 0x000103337649 ClangdTests` clang::clangd::(anonymous
>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
>> PB=0x7ffeefbfc658)  + 169 at ParsedAST.cpp:142
>> frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous
>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
>> PB=0x7ffeefbfc658)  + 77 at ParsedAST.cpp:139
>> frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous
>> namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678,
>> Clang=0x000114f33ea0, PB=0x7ffeefbfc658)  + 183 at ParsedAST.cpp:126
>> frame #3: 0x000103334189 ClangdTests`
>> clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp",
>> Length = 20), Inputs=0x7ffeefbfdf98, CI=nullptr,
>> CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830,
>> Preamble=std::__1::shared_ptr> clang::clangd::PreambleData>::element_type @ 0x000114f3e728 strong=2
>> weak=1)  + 3897 at ParsedAST.cpp:385
>> frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous
>> namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090)
>> + 1778 at ParsedASTTests.cpp:477
>>
>> This is the failed assert:
>>
>>   * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous
>> namespace)::ReplayPreamble::replay(this=0x000114f45da0)  + 556 at
>> ParsedAST.cpp:182
>> frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous
>> namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID =
>> 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2))  + 156 at
>> ParsedAST.cpp:166
>> frame #6: 0x000101b7900a ClangdTests`
>> clang::PPChainedCallbacks::FileChanged(this=0x000116204080, Loc=(ID =
>> 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 90 at
>> PPCallbacks.h:390
>> frame #7: 0x000101b79046 ClangdTests`
>> 

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

[Kadir Çetinkaya via cfe-commits]

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: 0x000103337649 ClangdTests` clang::clangd::(anonymous
> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
> PB=0x7ffeefbfc658)  + 169 at ParsedAST.cpp:142
> frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous
> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0,
> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80,
> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0,
> PB=0x7ffeefbfc658)  + 77 at ParsedAST.cpp:139
> frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous
> namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678,
> Clang=0x000114f33ea0, PB=0x7ffeefbfc658)  + 183 at ParsedAST.cpp:126
> frame #3: 0x000103334189 ClangdTests`
> clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp",
> Length = 20), Inputs=0x7ffeefbfdf98, CI=nullptr,
> CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830,
> Preamble=std::__1::shared_ptr clang::clangd::PreambleData>::element_type @ 0x000114f3e728 strong=2
> weak=1)  + 3897 at ParsedAST.cpp:385
> frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous
> namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090)
> + 1778 at ParsedASTTests.cpp:477
>
> This is the failed assert:
>
>   * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous
> namespace)::ReplayPreamble::replay(this=0x000114f45da0)  + 556 at
> ParsedAST.cpp:182
> frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous
> namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID =
> 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2))  + 156 at
> ParsedAST.cpp:166
> frame #6: 0x000101b7900a ClangdTests`
> clang::PPChainedCallbacks::FileChanged(this=0x000116204080, Loc=(ID =
> 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 90 at
> PPCallbacks.h:390
> frame #7: 0x000101b79046 ClangdTests`
> clang::PPChainedCallbacks::FileChanged(this=0x0001162040c0, Loc=(ID =
> 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at
> PPCallbacks.h:391
> frame #8: 0x000101b79046 ClangdTests`
> clang::PPChainedCallbacks::FileChanged(this=0x000116204100, Loc=(ID =
> 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at
> PPCallbacks.h:391
> frame #9: 

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

[Jan Korous via Phabricator via cfe-commits]

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: 0x000103337649 ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, 
Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, 
SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, 
PB=0x7ffeefbfc658)  + 169 at ParsedAST.cpp:142
frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, 
Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, 
SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, 
PB=0x7ffeefbfc658)  + 77 at ParsedAST.cpp:139
frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678, 
Clang=0x000114f33ea0, PB=0x7ffeefbfc658)  + 183 at ParsedAST.cpp:126
frame #3: 0x000103334189 ClangdTests` 
clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", Length 
= 20), Inputs=0x7ffeefbfdf98, CI=nullptr, 
CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830, 
Preamble=std::__1::shared_ptr::element_type 
@ 0x000114f3e728 strong=2 weak=1)  + 3897 at ParsedAST.cpp:385
frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous 
namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090)
  + 1778 at ParsedASTTests.cpp:477

This is the failed assert:

  * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::replay(this=0x000114f45da0)  + 556 at 
ParsedAST.cpp:182
frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID = 74), 
Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2))  + 156 at ParsedAST.cpp:166
frame #6: 0x000101b7900a ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204080, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 90 at PPCallbacks.h:390
frame #7: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x0001162040c0, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #8: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204100, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #9: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204160, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #10: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116205480, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #11: 0x000101b9a590 ClangdTests` 
clang::Preprocessor::HandleEndOfFile(this=0x000115850218, 
Result=0x000116808210, isEndOfMacro=false)  + 3904 at PPLexerChange.cpp:469
frame #12: 0x000101b38713 ClangdTests` 
clang::Lexer::LexEndOfFile(this=0x000116206330, Result=0x000116808210, 
CurPtr="")  + 931 at Lexer.cpp:2833
frame #13: 0x000101b39c44 ClangdTests` 
clang::Lexer::LexTokenInternal(this=0x000116206330, 
Result=0x000116808210, TokAtPhysicalStartOfLine=true)  + 420 at 
Lexer.cpp:3265
frame #14: 0x000101b382f8 ClangdTests` 
clang::Lexer::Lex(this=0x000116206330, Result=0x000116808210)  + 216 at 
Lexer.cpp:3216
frame #15: 0x000101bd7396 ClangdTests` 
clang::Preprocessor::Lex(this=0x000115850218, Result=0x000116808210)  + 
118 at Preprocessor.cpp:900
frame #16: 0x000101b75ef1 ClangdTests` 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb742eaa32121: [clangd] Handle additional includes while 
parsing ASTs (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Preamble.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -21,6 +22,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -30,51 +32,58 @@
 namespace clangd {
 namespace {
 
+MATCHER_P2(Distance, File, D, "") {
+  return arg.first() == File && arg.second == D;
+}
+
+std::shared_ptr
+createPreamble(llvm::StringRef Contents = "") {
+  auto TU = TestTU::withCode(Contents);
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  TU.Filename = "preamble.cpp";
+  auto PI = TU.inputs();
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
+  if (!CI) {
+ADD_FAILURE() << "failed to build compiler invocation";
+return nullptr;
+  }
+  if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr))
+return Preamble;
+  ADD_FAILURE() << "failed to build preamble";
+  return nullptr;
+}
+
 // Builds a preamble for BaselineContents, patches it for ModifiedContents and
 // returns the includes in the patch.
 IncludeStructure
 collectPatchedIncludes(llvm::StringRef ModifiedContents,
llvm::StringRef BaselineContents,
llvm::StringRef MainFileName = "main.cpp") {
-  std::string MainFile = testPath(MainFileName);
-  ParseInputs PI;
-  PI.FS = new llvm::vfs::InMemoryFileSystem;
-  MockCompilationDatabase CDB;
+  auto BaselinePreamble = createPreamble(BaselineContents);
+  // Create the patch.
+  auto TU = TestTU::withCode(ModifiedContents);
+  TU.Filename = MainFileName.str();
   // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
-  // Create invocation
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  auto PI = TU.inputs();
+  auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
+  // Collect patch contents.
   IgnoreDiagnostics Diags;
   auto CI = buildCompilerInvocation(PI, Diags);
-  assert(CI && "failed to create compiler invocation");
-  // Build baseline preamble.
-  PI.Contents = BaselineContents.str();
-  PI.Version = "baseline preamble";
-  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
-  assert(BaselinePreamble && "failed to build baseline preamble");
-  // Create the patch.
-  PI.Contents = ModifiedContents.str();
-  PI.Version = "modified contents";
-  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
-  // Collect patch contents.
   PP.apply(*CI);
-  llvm::StringRef PatchContents;
-  for (const auto  : CI->getPreprocessorOpts().RemappedFileBuffers) {
-if (Rempaped.first == testPath("__preamble_patch__.h")) {
-  PatchContents = Rempaped.second->getBuffer();
-  break;
-}
-  }
-  // Run preprocessor over the modified contents with patched Invocation to and
-  // BaselinePreamble to collect includes in the patch. We trim the input to
-  // only preamble section to not collect includes in the mainfile.
+  // Run preprocessor over the modified contents with patched Invocation. We
+  // provide a preamble and trim contents to ensure only the implicit header
+  // introduced by the patch is parsed and nothing else.
+  // We don't run PP directly over the patch cotents to test production
+  // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
   auto Clang =
   prepareCompilerInstance(std::move(CI), >Preamble,
   llvm::MemoryBuffer::getMemBufferCopy(
   ModifiedContents.slice(0, Bounds.Size).str()),
   PI.FS, Diags);
-  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
 ADD_FAILURE() << "failed begin source file";
@@ -163,6 +172,33 @@
   

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266215.
kadircet added a comment.

- Fix windows build bots in the face of `#import` directives


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Preamble.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -21,6 +22,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -30,51 +32,58 @@
 namespace clangd {
 namespace {
 
+MATCHER_P2(Distance, File, D, "") {
+  return arg.first() == File && arg.second == D;
+}
+
+std::shared_ptr
+createPreamble(llvm::StringRef Contents = "") {
+  auto TU = TestTU::withCode(Contents);
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  TU.Filename = "preamble.cpp";
+  auto PI = TU.inputs();
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
+  if (!CI) {
+ADD_FAILURE() << "failed to build compiler invocation";
+return nullptr;
+  }
+  if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr))
+return Preamble;
+  ADD_FAILURE() << "failed to build preamble";
+  return nullptr;
+}
+
 // Builds a preamble for BaselineContents, patches it for ModifiedContents and
 // returns the includes in the patch.
 IncludeStructure
 collectPatchedIncludes(llvm::StringRef ModifiedContents,
llvm::StringRef BaselineContents,
llvm::StringRef MainFileName = "main.cpp") {
-  std::string MainFile = testPath(MainFileName);
-  ParseInputs PI;
-  PI.FS = new llvm::vfs::InMemoryFileSystem;
-  MockCompilationDatabase CDB;
+  auto BaselinePreamble = createPreamble(BaselineContents);
+  // Create the patch.
+  auto TU = TestTU::withCode(ModifiedContents);
+  TU.Filename = MainFileName.str();
   // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
-  // Create invocation
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  auto PI = TU.inputs();
+  auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
+  // Collect patch contents.
   IgnoreDiagnostics Diags;
   auto CI = buildCompilerInvocation(PI, Diags);
-  assert(CI && "failed to create compiler invocation");
-  // Build baseline preamble.
-  PI.Contents = BaselineContents.str();
-  PI.Version = "baseline preamble";
-  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
-  assert(BaselinePreamble && "failed to build baseline preamble");
-  // Create the patch.
-  PI.Contents = ModifiedContents.str();
-  PI.Version = "modified contents";
-  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
-  // Collect patch contents.
   PP.apply(*CI);
-  llvm::StringRef PatchContents;
-  for (const auto  : CI->getPreprocessorOpts().RemappedFileBuffers) {
-if (Rempaped.first == testPath("__preamble_patch__.h")) {
-  PatchContents = Rempaped.second->getBuffer();
-  break;
-}
-  }
-  // Run preprocessor over the modified contents with patched Invocation to and
-  // BaselinePreamble to collect includes in the patch. We trim the input to
-  // only preamble section to not collect includes in the mainfile.
+  // Run preprocessor over the modified contents with patched Invocation. We
+  // provide a preamble and trim contents to ensure only the implicit header
+  // introduced by the patch is parsed and nothing else.
+  // We don't run PP directly over the patch cotents to test production
+  // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
   auto Clang =
   prepareCompilerInstance(std::move(CI), >Preamble,
   llvm::MemoryBuffer::getMemBufferCopy(
   ModifiedContents.slice(0, Bounds.Size).str()),
   PI.FS, Diags);
-  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
 ADD_FAILURE() << "failed begin source file";
@@ -163,6 +172,33 @@
   EXPECT_THAT(Includes, ElementsAre(AllOf(Field(::Written, 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266174.
kadircet added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Preamble.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -21,6 +22,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -30,51 +32,56 @@
 namespace clangd {
 namespace {
 
+MATCHER_P2(Distance, File, D, "") {
+  return arg.first() == File && arg.second == D;
+}
+
+std::shared_ptr
+createPreamble(llvm::StringRef Contents = "") {
+  auto TU = TestTU::withCode(Contents);
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  TU.Filename = "preamble.cpp";
+  auto PI = TU.inputs();
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
+  if (!CI) {
+ADD_FAILURE() << "failed to build compiler invocation";
+return nullptr;
+  }
+  if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr))
+return Preamble;
+  ADD_FAILURE() << "failed to build preamble";
+  return nullptr;
+}
+
 // Builds a preamble for BaselineContents, patches it for ModifiedContents and
 // returns the includes in the patch.
 IncludeStructure
 collectPatchedIncludes(llvm::StringRef ModifiedContents,
llvm::StringRef BaselineContents,
llvm::StringRef MainFileName = "main.cpp") {
-  std::string MainFile = testPath(MainFileName);
-  ParseInputs PI;
-  PI.FS = new llvm::vfs::InMemoryFileSystem;
-  MockCompilationDatabase CDB;
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
-  // Create invocation
-  IgnoreDiagnostics Diags;
-  auto CI = buildCompilerInvocation(PI, Diags);
-  assert(CI && "failed to create compiler invocation");
-  // Build baseline preamble.
-  PI.Contents = BaselineContents.str();
-  PI.Version = "baseline preamble";
-  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
-  assert(BaselinePreamble && "failed to build baseline preamble");
+  auto BaselinePreamble = createPreamble(BaselineContents);
   // Create the patch.
-  PI.Contents = ModifiedContents.str();
-  PI.Version = "modified contents";
-  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
+  auto TU = TestTU::withCode(ModifiedContents);
+  TU.Filename = MainFileName.str();
+  auto PI = TU.inputs();
+  auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
   // Collect patch contents.
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
   PP.apply(*CI);
-  llvm::StringRef PatchContents;
-  for (const auto  : CI->getPreprocessorOpts().RemappedFileBuffers) {
-if (Rempaped.first == testPath("__preamble_patch__.h")) {
-  PatchContents = Rempaped.second->getBuffer();
-  break;
-}
-  }
-  // Run preprocessor over the modified contents with patched Invocation to and
-  // BaselinePreamble to collect includes in the patch. We trim the input to
-  // only preamble section to not collect includes in the mainfile.
+  // Run preprocessor over the modified contents with patched Invocation. We
+  // provide a preamble and trim contents to ensure only the implicit header
+  // introduced by the patch is parsed and nothing else.
+  // We don't run PP directly over the patch cotents to test production
+  // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
   auto Clang =
   prepareCompilerInstance(std::move(CI), >Preamble,
   llvm::MemoryBuffer::getMemBufferCopy(
   ModifiedContents.slice(0, Bounds.Size).str()),
   PI.FS, Diags);
-  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
 ADD_FAILURE() << "failed begin source file";
@@ -163,6 +170,33 @@
   EXPECT_THAT(Includes, ElementsAre(AllOf(Field(::Written, ""),
   Field(::HashLine, 0;
 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry for the delay. I think this is good though I have a nagging feeling I 
don't understand all the implications yet :-)


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 261745.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Preamble.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -21,6 +22,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -30,49 +32,53 @@
 namespace clangd {
 namespace {
 
+MATCHER_P2(Distance, File, D, "") {
+  return arg.first() == File && arg.second == D;
+}
+
+std::shared_ptr
+createPreamble(llvm::StringRef Contents = "") {
+  auto TU = TestTU::withCode(Contents);
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  TU.Filename = "preamble.cpp";
+  auto PI = TU.inputs();
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
+  if (!CI) {
+ADD_FAILURE() << "failed to build compiler invocation";
+return nullptr;
+  }
+  if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr))
+return Preamble;
+  ADD_FAILURE() << "failed to build preamble";
+  return nullptr;
+}
+
 // Builds a preamble for BaselineContents, patches it for ModifiedContents and
 // returns the includes in the patch.
 IncludeStructure collectPatchedIncludes(llvm::StringRef ModifiedContents,
 llvm::StringRef BaselineContents) {
-  std::string MainFile = testPath("main.cpp");
-  ParseInputs PI;
-  PI.FS = new llvm::vfs::InMemoryFileSystem;
-  MockCompilationDatabase CDB;
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
-  // Create invocation
-  IgnoreDiagnostics Diags;
-  auto CI = buildCompilerInvocation(PI, Diags);
-  assert(CI && "failed to create compiler invocation");
-  // Build baseline preamble.
-  PI.Contents = BaselineContents.str();
-  PI.Version = "baseline preamble";
-  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
-  assert(BaselinePreamble && "failed to build baseline preamble");
+  auto BaselinePreamble = createPreamble(BaselineContents);
   // Create the patch.
-  PI.Contents = ModifiedContents.str();
-  PI.Version = "modified contents";
-  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
+  auto TU = TestTU::withCode(ModifiedContents);
+  auto PI = TU.inputs();
+  auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
   // Collect patch contents.
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
   PP.apply(*CI);
-  llvm::StringRef PatchContents;
-  for (const auto  : CI->getPreprocessorOpts().RemappedFileBuffers) {
-if (Rempaped.first == testPath("__preamble_patch__.h")) {
-  PatchContents = Rempaped.second->getBuffer();
-  break;
-}
-  }
-  // Run preprocessor over the modified contents with patched Invocation to and
-  // BaselinePreamble to collect includes in the patch. We trim the input to
-  // only preamble section to not collect includes in the mainfile.
+  // Run preprocessor over the modified contents with patched Invocation. We
+  // provide a preamble and trim contents to ensure only the implicit header
+  // introduced by the patch is parsed and nothing else.
+  // We don't run PP directly over the patch cotents to test production
+  // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
   auto Clang =
   prepareCompilerInstance(std::move(CI), >Preamble,
   llvm::MemoryBuffer::getMemBufferCopy(
   ModifiedContents.slice(0, Bounds.Size).str()),
   PI.FS, Diags);
-  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
 ADD_FAILURE() << "failed begin source file";
@@ -150,6 +156,32 @@
   Field(::Written, "")));
 }
 
+TEST(PreamblePatchTest, PatchesPreambleIncludes) {
+  IgnoreDiagnostics Diags;
+  auto TU = 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 8 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:280
 const PreambleData ) {
+  assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
   // First scan the include directives in Baseline and Modified. These will be

sammccall wrote:
> BTW do we want to check isPreambleCompatible and bail out early if it is, or 
> is that the caller's job?
I am planning to leave it to the caller. Eventually this PreamblePatch should 
be generated by TUScheduler, in which we can check for staleness and issue 
either an unmodified patch or create a patch and use it until new preamble is 
ready or patch is invalidated.



Comment at: clang-tools-extra/clangd/Preamble.cpp:332
+  for (auto  : *ModifiedIncludes) {
+auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
+// Include already present in the baseline preamble. Set resolved path and

sammccall wrote:
> This explicitly looks at the existing includes based on spelling, and not 
> whether the include was resolved during scanning or not.
> 
> One implication is that if you insert an #include that was already 
> transitively included (which IIUC will hit the preamble VFS cache and thus be 
> resolved) then we're not going to take advantage of this to record its 
> resolution now.
> 
> Instead we're going to parse it along with the mainfile, and... well, I 
> *hope* everything works out:
>  - do we pay for the stat, or is the preamble's stat cache still in effect?
>  - if the file is include-guarded, we're not going to read or parse it I 
> guess?
>  - if it's not include-guarded but #imported, then again we won't re-read or 
> re-parse it because we use the same directive?
>  - if the file isn't include-guarded, I guess we must read it (because 
> preamble doesn't contain the actual buffer) and then parse it, but such is 
> life. 
> 
> This explicitly looks at the existing includes based on spelling, and not 
> whether the include was resolved during scanning or not.

Right. Scanning won't resolve anything as we pass an empty FS to preprocessor.

> Instead we're going to parse it along with the mainfile, and... well, I 
> *hope* everything works out:
> do we pay for the stat, or is the preamble's stat cache still in effect?

It depends on the user of PreamblePatch, while building an AST we build 
preprocessor with cached fs coming from preamble.

> if the file is include-guarded, we're not going to read or parse it I guess?

Yes that's the case. We'll only see the include directive.

> if it's not include-guarded but #imported, then again we won't re-read or 
> re-parse it because we use the same directive?

Yes again.

> if the file isn't include-guarded, I guess we must read it (because preamble 
> doesn't contain the actual buffer) and then parse it, but such is life.

Unfortunately yes again, but as you said i am not sure if that's a case we 
should try and recover from as i can only see two use cases:
- You include the header in a different preprocessor state and intentionally 
want it to parse again, hopefully we'll handle this after macro patching.
- You deleted the include from a transitively included file and inserted it 
back into this file again. Well, now the hell breaks loose until we build the 
preamble again. In the worst case AST will turn into a garbage and we won't be 
able to serve anything but code completions, which is the state of the world 
without preamble patching, we are just consuming more cpu now.



Comment at: clang-tools-extra/clangd/Preamble.cpp:340
 }
 // FIXME: Traverse once
 auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);

sammccall wrote:
> nice to have a comment similar to the one on line 333 for this case
> ```
> // Include is new in the modified preamble.
> // Inject it into the patch and use #line to set the presumed location to 
> where it is spelled
> ```
handled in D78743


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is excellent, a pretty nice model in the end!
Good job puzzling it out, when we discussed the idea in the past I imagined 
this part would be a pile of hacks.

Thanks for the offline help understanding this, too...




Comment at: clang-tools-extra/clangd/ParsedAST.cpp:268
 
+  PreamblePatch Patch = Preamble
+? PreamblePatch::create(Filename, Inputs, 
*Preamble)

As discussed offline, might be clearer not to have the patch exist unless the 
preamble does.



Comment at: clang-tools-extra/clangd/ParsedAST.h:51
   static llvm::Optional
-  build(llvm::StringRef Version, std::unique_ptr CI,
+  build(const ParseInputs ,
+std::unique_ptr CI,

ah, this is going to conflict with 
https://github.com/llvm/llvm-project/commit/6880c4dfa3981ec26d44b5f4844b641342a4e3e8
Sorry about that, but that change also ParseInputs available here so I think 
you can just drop your changes.



Comment at: clang-tools-extra/clangd/Preamble.cpp:280
 const PreambleData ) {
+  assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
   // First scan the include directives in Baseline and Modified. These will be

BTW do we want to check isPreambleCompatible and bail out early if it is, or is 
that the caller's job?



Comment at: clang-tools-extra/clangd/Preamble.cpp:332
+  for (auto  : *ModifiedIncludes) {
+auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
+// Include already present in the baseline preamble. Set resolved path and

This explicitly looks at the existing includes based on spelling, and not 
whether the include was resolved during scanning or not.

One implication is that if you insert an #include that was already transitively 
included (which IIUC will hit the preamble VFS cache and thus be resolved) then 
we're not going to take advantage of this to record its resolution now.

Instead we're going to parse it along with the mainfile, and... well, I *hope* 
everything works out:
 - do we pay for the stat, or is the preamble's stat cache still in effect?
 - if the file is include-guarded, we're not going to read or parse it I guess?
 - if it's not include-guarded but #imported, then again we won't re-read or 
re-parse it because we use the same directive?
 - if the file isn't include-guarded, I guess we must read it (because preamble 
doesn't contain the actual buffer) and then parse it, but such is life. 




Comment at: clang-tools-extra/clangd/Preamble.cpp:340
 }
 // FIXME: Traverse once
 auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);

nice to have a comment similar to the one on line 333 for this case
```
// Include is new in the modified preamble.
// Inject it into the patch and use #line to set the presumed location to where 
it is spelled
```



Comment at: clang-tools-extra/clangd/Preamble.h:112
 
+  /// Adjusts \p BaselineIncludes to reflect state of \p Modified. This won't
+  /// add any new includes but rather drop the deleted ones. Note that it won't

This comment explains what but not why, and the why is pretty non-obvious here.
Assuming we switch to returning a vector, I'd call it `preambleIncludes()` and 
say something like:

```
Returns #include directives from the \c Modified preamble that were resolved 
using the \c Baseline preamble.
This covers the new locations of inclusions that were moved around, but not 
inclusions of new files.
Those will be recorded when parsing the main file: the includes in the injected 
section will be resolved back to their spelled positions in the main file using 
the presumed-location mechanism.
```

("injected section" isn't a term we've defined anywhere, it'd be good to give 
this idea some name in the class comment as it's the main mechanism behind 
preamble patching)



Comment at: clang-tools-extra/clangd/Preamble.h:115
+  /// mutate include depths.
+  void patchPreambleIncludes(IncludeStructure ) const;
+

kadircet wrote:
> i am not happy about this interface. I was thinking about a 
> `std::vector remainingBaselineIncludes() const` but didn't go for 
> it to keep the concept of `patching`.
> 
> I suppose the former is more intuitive though, WDYT?
Looking at the callsite, I think I like returning a vector better. buildAST is 
copyng a data structure specifically to be patched, and the patch is just 
overwriting it. The interface looks good but doesn't really reflect what's 
going on.

To make the vector thing work cleanly, I think we should have it always 
overwrite. This implies it needs to have the preamble's includes, so the 
default constructor needs to go away. I'd suggest adding a static factory 
`PreamblePatch::unmodified(const PreambleData&)` and hiding all 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.h:115
+  /// mutate include depths.
+  void patchPreambleIncludes(IncludeStructure ) const;
+

i am not happy about this interface. I was thinking about a 
`std::vector remainingBaselineIncludes() const` but didn't go for it 
to keep the concept of `patching`.

I suppose the former is more intuitive though, WDYT?


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

this is ready for review now, to explain the flow:

While creating a patch we also record remaining includes from Baseline left in 
Modified, as the new ones will be put into the patch already.
That way ParsedAST can keep track of latest state of the preamble + main file 
includes, with the patched includes ending up in the latter.

It doesn't modify include depths, as it is only used by code completion. If 
need be we can prune the tree to get rid of deleted includes.


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 260228.
kadircet added a comment.

- Use PreamblePatch
- Add more tests
- Handle deleted includes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Preamble.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -21,6 +22,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -30,49 +32,53 @@
 namespace clangd {
 namespace {
 
+MATCHER_P2(Distance, File, D, "") {
+  return arg.first() == File && arg.second == D;
+}
+
+std::shared_ptr
+createPreamble(llvm::StringRef Contents = "") {
+  auto TU = TestTU::withCode(Contents);
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  TU.ExtraArgs = {"-fno-ms-compatibility"};
+  TU.Filename = "preamble.cpp";
+  auto PI = TU.inputs();
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
+  if (!CI) {
+ADD_FAILURE() << "failed to build compiler invocation";
+return nullptr;
+  }
+  if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr))
+return Preamble;
+  ADD_FAILURE() << "failed to build preamble";
+  return nullptr;
+}
+
 // Builds a preamble for BaselineContents, patches it for ModifiedContents and
 // returns the includes in the patch.
 IncludeStructure collectPatchedIncludes(llvm::StringRef ModifiedContents,
 llvm::StringRef BaselineContents) {
-  std::string MainFile = testPath("main.cpp");
-  ParseInputs PI;
-  PI.FS = new llvm::vfs::InMemoryFileSystem;
-  MockCompilationDatabase CDB;
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
-  // Create invocation
-  IgnoreDiagnostics Diags;
-  auto CI = buildCompilerInvocation(PI, Diags);
-  assert(CI && "failed to create compiler invocation");
-  // Build baseline preamble.
-  PI.Contents = BaselineContents.str();
-  PI.Version = "baseline preamble";
-  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
-  assert(BaselinePreamble && "failed to build baseline preamble");
+  auto BaselinePreamble = createPreamble(BaselineContents);
   // Create the patch.
-  PI.Contents = ModifiedContents.str();
-  PI.Version = "modified contents";
-  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
+  auto TU = TestTU::withCode(ModifiedContents);
+  auto PI = TU.inputs();
+  auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
   // Collect patch contents.
+  IgnoreDiagnostics Diags;
+  auto CI = buildCompilerInvocation(PI, Diags);
   PP.apply(*CI);
-  llvm::StringRef PatchContents;
-  for (const auto  : CI->getPreprocessorOpts().RemappedFileBuffers) {
-if (Rempaped.first == testPath("__preamble_patch__.h")) {
-  PatchContents = Rempaped.second->getBuffer();
-  break;
-}
-  }
-  // Run preprocessor over the modified contents with patched Invocation to and
-  // BaselinePreamble to collect includes in the patch. We trim the input to
-  // only preamble section to not collect includes in the mainfile.
+  // Run preprocessor over the modified contents with patched Invocation. We
+  // provide a preamble and trim contents to ensure only the implicit header
+  // introduced by the patch is parsed and nothing else.
+  // We don't run PP directly over the patch cotents to test production
+  // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
   auto Clang =
   prepareCompilerInstance(std::move(CI), >Preamble,
   llvm::MemoryBuffer::getMemBufferCopy(
   ModifiedContents.slice(0, Bounds.Size).str()),
   PI.FS, Diags);
-  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
 ADD_FAILURE() << "failed begin source file";
@@ -150,6 +156,38 @@
   Field(::Written, "")));
 }
 
+TEST(PreamblePatchTest, 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet planned changes to this revision.
kadircet added a comment.

this doesn't handle source locations correctly in presence of deleted headers.


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, mgrang, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.

Depends on D77392 .

Enables building ASTs with stale preambles by handling additional preamble
includes. Sets the correct location information for those imaginary includes so
that features like gotodef/documentlink keeps functioning propoerly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77644

Files:
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -17,7 +17,9 @@
 #include "Annotations.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
+#include "Headers.h"
 #include "ParsedAST.h"
+#include "Preamble.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
@@ -28,6 +30,7 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Token.h"
 #include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock-matchers.h"
@@ -82,6 +85,13 @@
   return arg.beginOffset() == R.Begin && arg.endOffset() == R.End;
 }
 
+MATCHER(EqInc, "") {
+  Inclusion Actual = testing::get<0>(arg);
+  Inclusion Expected = testing::get<1>(arg);
+  return std::tie(Actual.HashOffset, Actual.R, Actual.Written) ==
+ std::tie(Expected.HashOffset, Expected.R, Expected.Written);
+}
+
 TEST(ParsedASTTest, TopLevelDecls) {
   TestTU TU;
   TU.HeaderCode = R"(
@@ -420,6 +430,73 @@
   }
 }
 
+TEST(ParsedASTTest, AdditionalIncludes) {
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.AdditionalFiles["foo.h"] = "void foo();";
+  TU.AdditionalFiles["sub/baz.h"] = "void baz();";
+  TU.AdditionalFiles["sub/aux.h"] = "void aux();";
+  TU.ExtraArgs = {"-I" + testPath("sub")};
+  TU.Code = R"cpp(
+#include "baz.h"
+#include "foo.h"
+#include "sub/aux.h"
+void bar() {
+  foo();
+  baz();
+  aux();
+})cpp";
+  auto ExpectedAST = TU.build();
+
+  // Build preamble with no includes.
+  TU.Code = R"cpp(
+void bar() {
+  foo();
+  baz();
+  aux();
+})cpp";
+  StoreDiags Diags;
+  auto Inputs = TU.inputs();
+  auto CI = buildCompilerInvocation(Inputs, Diags);
+  auto Preamble = buildPreamble("foo.cpp", *CI, Inputs, true, nullptr);
+  ASSERT_TRUE(Preamble);
+  EXPECT_THAT(Preamble->Includes.MainFileIncludes, testing::IsEmpty());
+
+  // Now build an AST using additional includes and check that locations are
+  // correctly parsed.
+  TU.Code = R"cpp(
+#include "baz.h"
+#include "foo.h"
+#include "sub/aux.h"
+void bar() {
+  foo();
+  baz();
+  aux();
+})cpp";
+  Inputs = TU.inputs();
+  Inputs.Opts.AdditionalIncludes =
+  getPreambleIncludes(TU.Code, ExpectedAST.getLangOpts());
+  auto PatchedAST = buildAST("foo.cpp", std::move(CI), {}, Inputs, Preamble);
+  ASSERT_TRUE(PatchedAST);
+
+  // Ensure source location information is correct.
+  EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
+  testing::Pointwise(
+  EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
+  auto StringMapToVector = [](const llvm::StringMap SM) {
+std::vector> Res;
+for (const auto  : SM)
+  Res.push_back({E.first().str(), E.second});
+llvm::sort(Res);
+return Res;
+  };
+  // Ensure file proximity signals are correct.
+  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
+testPath("foo.cpp"))),
+StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
+testPath("foo.cpp";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -262,6 +262,15 @@
   std::string Filename =
   std::string(Buffer->getBufferIdentifier()); // Absolute.
 
+  // Process additional includes as implicit includes to ensure AST is built
+  // with necessary symbol information.
+  for (const auto  : Opts.AdditionalIncludes) {
+// Inc.Written contains quotes or angles, preprocessor requires them to be
+// stripped.
+CI->getPreprocessorOpts().Includes.emplace_back(
+llvm::StringRef(Inc.Written).drop_back().drop_front());
+  }
+
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
std::move(Buffer), VFS, ASTDiags);
   if (!Clang)
@@ -431,6 +440,31 @@
 std::vector D =