[llvm-branch-commits] [clang-tools-extra-branch] r368133 - Merging r368019:
Hans Wennborg via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Wed Aug 7 01:31:37 PDT 2019
Author: hans
Date: Wed Aug 7 01:31:37 2019
New Revision: 368133
URL: http://llvm.org/viewvc/llvm-project?rev=368133&view=rev
Log:
Merging r368019:
------------------------------------------------------------------------
r368019 | ibiryukov | 2019-08-06 13:37:50 +0200 (Tue, 06 Aug 2019) | 28 lines
[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/branches/release_90/ (props changed)
clang-tools-extra/branches/release_90/clangd/IncludeFixer.cpp
clang-tools-extra/branches/release_90/clangd/IncludeFixer.h
clang-tools-extra/branches/release_90/clangd/unittests/DiagnosticsTests.cpp
Propchange: clang-tools-extra/branches/release_90/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Aug 7 01:31:37 2019
@@ -1 +1 @@
-/clang-tools-extra/trunk:366443,366451,366455,366545,366559,366687,366811,366880,366900,366991-366992,367112,367687
+/clang-tools-extra/trunk:366443,366451,366455,366545,366559,366687,366811,366880,366900,366991-366992,367112,367687,368019
Modified: clang-tools-extra/branches/release_90/clangd/IncludeFixer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/branches/release_90/clangd/IncludeFixer.cpp?rev=368133&r1=368132&r2=368133&view=diff
==============================================================================
--- clang-tools-extra/branches/release_90/clangd/IncludeFixer.cpp (original)
+++ clang-tools-extra/branches/release_90/clangd/IncludeFixer.cpp Wed Aug 7 01:31:37 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 (!SemaPtr->SourceMgr.isWrittenInMainFile(Typo.getLoc()))
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/branches/release_90/clangd/IncludeFixer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/branches/release_90/clangd/IncludeFixer.h?rev=368133&r1=368132&r2=368133&view=diff
==============================================================================
--- clang-tools-extra/branches/release_90/clangd/IncludeFixer.h (original)
+++ clang-tools-extra/branches/release_90/clangd/IncludeFixer.h Wed Aug 7 01:31:37 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/branches/release_90/clangd/unittests/DiagnosticsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/branches/release_90/clangd/unittests/DiagnosticsTests.cpp?rev=368133&r1=368132&r2=368133&view=diff
==============================================================================
--- clang-tools-extra/branches/release_90/clangd/unittests/DiagnosticsTests.cpp (original)
+++ clang-tools-extra/branches/release_90/clangd/unittests/DiagnosticsTests.cpp Wed Aug 7 01:31:37 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"]]
More information about the llvm-branch-commits
mailing list