+Hans Wennborg <hwennb...@google.com>, 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
