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