<div dir="ltr">Many thanks!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 7, 2019 at 10:31 AM Hans Wennborg <<a href="mailto:hwennborg@google.com">hwennborg@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Yes, merged in r368133.<br>
<br>
On Tue, Aug 6, 2019 at 4:33 PM Ilya Biryukov <<a href="mailto:ibiryukov@google.com" target="_blank">ibiryukov@google.com</a>> wrote:<br>
><br>
> +Hans Wennborg, could we merge this into the release?<br>
><br>
> On Tue, Aug 6, 2019 at 1:36 PM Ilya Biryukov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: ibiryukov<br>
>> Date: Tue Aug 6 04:37:50 2019<br>
>> New Revision: 368019<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=368019&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=368019&view=rev</a><br>
>> Log:<br>
>> [clangd] Compute scopes eagerly in IncludeFixer<br>
>><br>
>> Summary:<br>
>> Computing lazily leads to crashes. In particular, computing scopes may<br>
>> produce diagnostics (from inside template instantiations) and we<br>
>> currently do it when processing another diagnostic, which leads to<br>
>> crashes.<br>
>><br>
>> Moreover, we remember and access 'Scope*' when computing scopes. This<br>
>> might lead to invalid memory access if the Scope is deleted by the time<br>
>> we run the delayed computation. We did not actually construct an example<br>
>> when this happens, though.<br>
>><br>
>> From the VCS and review history, it seems the optimization was<br>
>> introduced in the initial version without a mention of any performance<br>
>> benchmarks justifying the performance gains. This led me to a<br>
>> conclusion that the optimization was premature, so removing it to avoid<br>
>> crashes seems like the right trade-off at that point.<br>
>><br>
>> Reviewers: sammccall<br>
>><br>
>> Reviewed By: sammccall<br>
>><br>
>> Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits<br>
>><br>
>> Tags: #clang<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D65796" rel="noreferrer" target="_blank">https://reviews.llvm.org/D65796</a><br>
>><br>
>> Modified:<br>
>> clang-tools-extra/trunk/clangd/IncludeFixer.cpp<br>
>> clang-tools-extra/trunk/clangd/IncludeFixer.h<br>
>> clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp<br>
>><br>
>> Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff</a><br>
>> ==============================================================================<br>
>> --- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)<br>
>> +++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Tue Aug 6 04:37:50 2019<br>
>> @@ -16,6 +16,7 @@<br>
>> #include "index/Symbol.h"<br>
>> #include "clang/AST/Decl.h"<br>
>> #include "clang/AST/DeclBase.h"<br>
>> +#include "clang/AST/DeclarationName.h"<br>
>> #include "clang/AST/NestedNameSpecifier.h"<br>
>> #include "clang/AST/Type.h"<br>
>> #include "clang/Basic/Diagnostic.h"<br>
>> @@ -34,6 +35,7 @@<br>
>> #include "llvm/ADT/DenseMap.h"<br>
>> #include "llvm/ADT/None.h"<br>
>> #include "llvm/ADT/Optional.h"<br>
>> +#include "llvm/ADT/StringExtras.h"<br>
>> #include "llvm/ADT/StringRef.h"<br>
>> #include "llvm/ADT/StringSet.h"<br>
>> #include "llvm/Support/Error.h"<br>
>> @@ -301,6 +303,24 @@ llvm::Optional<CheapUnresolvedName> extr<br>
>> return Result;<br>
>> }<br>
>><br>
>> +/// Returns all namespace scopes that the unqualified lookup would visit.<br>
>> +std::vector<std::string><br>
>> +collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,<br>
>> + Sema::LookupNameKind LookupKind) {<br>
>> + std::vector<std::string> Scopes;<br>
>> + VisitedContextCollector Collector;<br>
>> + Sem.LookupVisibleDecls(S, LookupKind, Collector,<br>
>> + /*IncludeGlobalScope=*/false,<br>
>> + /*LoadExternal=*/false);<br>
>> +<br>
>> + Scopes.push_back("");<br>
>> + for (const auto *Ctx : Collector.takeVisitedContexts()) {<br>
>> + if (isa<NamespaceDecl>(Ctx))<br>
>> + Scopes.push_back(printNamespaceScope(*Ctx));<br>
>> + }<br>
>> + return Scopes;<br>
>> +}<br>
>> +<br>
>> class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {<br>
>> public:<br>
>> UnresolvedNameRecorder(llvm::Optional<UnresolvedName> &LastUnresolvedName)<br>
>> @@ -321,48 +341,30 @@ public:<br>
>> if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))<br>
>> return clang::TypoCorrection();<br>
>><br>
>> - // This is not done lazily because `SS` can get out of scope and it's<br>
>> - // relatively cheap.<br>
>> auto Extracted = extractUnresolvedNameCheaply(<br>
>> SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,<br>
>> static_cast<Sema::LookupNameKind>(LookupKind) ==<br>
>> Sema::LookupNameKind::LookupNestedNameSpecifierName);<br>
>> if (!Extracted)<br>
>> return TypoCorrection();<br>
>> - auto CheapUnresolved = std::move(*Extracted);<br>
>> +<br>
>> UnresolvedName Unresolved;<br>
>> - Unresolved.Name = CheapUnresolved.Name;<br>
>> + Unresolved.Name = Extracted->Name;<br>
>> Unresolved.Loc = Typo.getBeginLoc();<br>
>> -<br>
>> - if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.<br>
>> + if (!Extracted->ResolvedScope && !S) // Give up if no scope available.<br>
>> return TypoCorrection();<br>
>><br>
>> - auto *Sem = SemaPtr; // Avoid capturing `this`.<br>
>> - Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {<br>
>> - std::vector<std::string> Scopes;<br>
>> - if (CheapUnresolved.ResolvedScope) {<br>
>> - Scopes.push_back(*CheapUnresolved.ResolvedScope);<br>
>> - } else {<br>
>> - assert(S);<br>
>> - // No scope specifier is specified. Collect all accessible scopes in the<br>
>> - // context.<br>
>> - VisitedContextCollector Collector;<br>
>> - Sem->LookupVisibleDecls(<br>
>> - S, static_cast<Sema::LookupNameKind>(LookupKind), Collector,<br>
>> - /*IncludeGlobalScope=*/false,<br>
>> - /*LoadExternal=*/false);<br>
>> -<br>
>> - Scopes.push_back("");<br>
>> - for (const auto *Ctx : Collector.takeVisitedContexts())<br>
>> - if (isa<NamespaceDecl>(Ctx))<br>
>> - Scopes.push_back(printNamespaceScope(*Ctx));<br>
>> - }<br>
>> + if (Extracted->ResolvedScope)<br>
>> + Unresolved.Scopes.push_back(*Extracted->ResolvedScope);<br>
>> + else // no qualifier or qualifier is unresolved.<br>
>> + Unresolved.Scopes = collectAccessibleScopes(<br>
>> + *SemaPtr, Typo, S, static_cast<Sema::LookupNameKind>(LookupKind));<br>
>> +<br>
>> + if (Extracted->UnresolvedScope) {<br>
>> + for (std::string &Scope : Unresolved.Scopes)<br>
>> + Scope += *Extracted->UnresolvedScope;<br>
>> + }<br>
>><br>
>> - if (CheapUnresolved.UnresolvedScope)<br>
>> - for (auto &Scope : Scopes)<br>
>> - Scope.append(*CheapUnresolved.UnresolvedScope);<br>
>> - return Scopes;<br>
>> - };<br>
>> LastUnresolvedName = std::move(Unresolved);<br>
>><br>
>> // Never return a valid correction to try to recover. Our suggested fixes<br>
>> @@ -384,14 +386,13 @@ IncludeFixer::unresolvedNameRecorder() {<br>
>> std::vector<Fix> IncludeFixer::fixUnresolvedName() const {<br>
>> assert(LastUnresolvedName.hasValue());<br>
>> auto &Unresolved = *LastUnresolvedName;<br>
>> - std::vector<std::string> Scopes = Unresolved.GetScopes();<br>
>> vlog("Trying to fix unresolved name \"{0}\" in scopes: [{1}]",<br>
>> - Unresolved.Name, llvm::join(Scopes.begin(), Scopes.end(), ", "));<br>
>> + Unresolved.Name, llvm::join(Unresolved.Scopes, ", "));<br>
>><br>
>> FuzzyFindRequest Req;<br>
>> Req.AnyScope = false;<br>
>> Req.Query = Unresolved.Name;<br>
>> - Req.Scopes = Scopes;<br>
>> + Req.Scopes = Unresolved.Scopes;<br>
>> Req.RestrictForCodeCompletion = true;<br>
>> Req.Limit = 100;<br>
>><br>
>><br>
>> Modified: clang-tools-extra/trunk/clangd/IncludeFixer.h<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.h?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.h?rev=368019&r1=368018&r2=368019&view=diff</a><br>
>> ==============================================================================<br>
>> --- clang-tools-extra/trunk/clangd/IncludeFixer.h (original)<br>
>> +++ clang-tools-extra/trunk/clangd/IncludeFixer.h Tue Aug 6 04:37:50 2019<br>
>> @@ -58,9 +58,7 @@ private:<br>
>> struct UnresolvedName {<br>
>> std::string Name; // E.g. "X" in foo::X.<br>
>> SourceLocation Loc; // Start location of the unresolved name.<br>
>> - // Lazily get the possible scopes of the unresolved name. `Sema` must be<br>
>> - // alive when this is called.<br>
>> - std::function<std::vector<std::string>()> GetScopes;<br>
>> + std::vector<std::string> Scopes; // Namespace scopes we should search in.<br>
>> };<br>
>><br>
>> /// Records the last unresolved name seen by Sema.<br>
>><br>
>> Modified: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=368019&r1=368018&r2=368019&view=diff</a><br>
>> ==============================================================================<br>
>> --- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp (original)<br>
>> +++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp Tue Aug 6 04:37:50 2019<br>
>> @@ -797,6 +797,27 @@ namespace c {<br>
>> "Add include \"x.h\" for symbol a::X")))));<br>
>> }<br>
>><br>
>> +TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {<br>
>> + Annotations Test(R"cpp(<br>
>> + template <typename T> struct Templ {<br>
>> + template <typename U><br>
>> + typename U::type operator=(const U &);<br>
>> + };<br>
>> +<br>
>> + struct A {<br>
>> + Templ<char> s;<br>
>> + A() { [[a]]; } // this caused crashes if we compute scopes lazily.<br>
>> + };<br>
>> + )cpp");<br>
>> +<br>
>> + auto TU = TestTU::withCode(Test.code());<br>
>> + auto Index = buildIndexWithSymbol({});<br>
>> + TU.ExternalIndex = Index.get();<br>
>> +<br>
>> + EXPECT_THAT(TU.build().getDiagnostics(),<br>
>> + ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));<br>
>> +}<br>
>> +<br>
>> TEST(DiagsInHeaders, DiagInsideHeader) {<br>
>> Annotations Main(R"cpp(<br>
>> #include [["a.h"]]<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
> --<br>
> Regards,<br>
> Ilya Biryukov<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>