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