[clang-tools-extra] r368019 - [clangd] Compute scopes eagerly in IncludeFixer
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 7 01:30:49 PDT 2019
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
More information about the cfe-commits
mailing list