[clang-tools-extra] r368019 - [clangd] Compute scopes eagerly in IncludeFixer
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 6 07:33:20 PDT 2019
+Hans Wennborg <hwennborg at 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 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190806/32c1ecbb/attachment-0001.html>
More information about the cfe-commits
mailing list