Many thanks! On Wed, Aug 7, 2019 at 10:31 AM Hans Wennborg <hwennb...@google.com> wrote:
> 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 > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
