[clang-tools-extra] r368019 - [clangd] Compute scopes eagerly in IncludeFixer
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 7 01:44:44 PDT 2019
Many thanks!
On Wed, Aug 7, 2019 at 10:31 AM Hans Wennborg <hwennborg at google.com> wrote:
> Yes, merged in r368133.
>
> On Tue, Aug 6, 2019 at 4:33 PM Ilya Biryukov <ibiryukov at 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 at 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 at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
> >
> > --
> > Regards,
> > Ilya Biryukov
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190807/c5e65073/attachment-0001.html>
More information about the cfe-commits
mailing list