<div dir="ltr"><a class="gmail_plusreply" id="m_-2625572906849018657plusReplyChip-7" href="mailto:hwennborg@google.com" target="_blank">+Hans Wennborg</a>, could we merge this into the release?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 6, 2019 at 1:36 PM Ilya Biryukov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: ibiryukov<br>
Date: Tue Aug 6 04:37:50 2019<br>
New Revision: 368019<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=368019&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=368019&view=rev</a><br>
Log:<br>
[clangd] Compute scopes eagerly in IncludeFixer<br>
<br>
Summary:<br>
Computing lazily leads to crashes. In particular, computing scopes may<br>
produce diagnostics (from inside template instantiations) and we<br>
currently do it when processing another diagnostic, which leads to<br>
crashes.<br>
<br>
Moreover, we remember and access 'Scope*' when computing scopes. This<br>
might lead to invalid memory access if the Scope is deleted by the time<br>
we run the delayed computation. We did not actually construct an example<br>
when this happens, though.<br>
<br>
>From the VCS and review history, it seems the optimization was<br>
introduced in the initial version without a mention of any performance<br>
benchmarks justifying the performance gains. This led me to a<br>
conclusion that the optimization was premature, so removing it to avoid<br>
crashes seems like the right trade-off at that point.<br>
<br>
Reviewers: sammccall<br>
<br>
Reviewed By: sammccall<br>
<br>
Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits<br>
<br>
Tags: #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D65796" rel="noreferrer" target="_blank">https://reviews.llvm.org/D65796</a><br>
<br>
Modified:<br>
clang-tools-extra/trunk/clangd/IncludeFixer.cpp<br>
clang-tools-extra/trunk/clangd/IncludeFixer.h<br>
clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Tue Aug 6 04:37:50 2019<br>
@@ -16,6 +16,7 @@<br>
#include "index/Symbol.h"<br>
#include "clang/AST/Decl.h"<br>
#include "clang/AST/DeclBase.h"<br>
+#include "clang/AST/DeclarationName.h"<br>
#include "clang/AST/NestedNameSpecifier.h"<br>
#include "clang/AST/Type.h"<br>
#include "clang/Basic/Diagnostic.h"<br>
@@ -34,6 +35,7 @@<br>
#include "llvm/ADT/DenseMap.h"<br>
#include "llvm/ADT/None.h"<br>
#include "llvm/ADT/Optional.h"<br>
+#include "llvm/ADT/StringExtras.h"<br>
#include "llvm/ADT/StringRef.h"<br>
#include "llvm/ADT/StringSet.h"<br>
#include "llvm/Support/Error.h"<br>
@@ -301,6 +303,24 @@ llvm::Optional<CheapUnresolvedName> extr<br>
return Result;<br>
}<br>
<br>
+/// Returns all namespace scopes that the unqualified lookup would visit.<br>
+std::vector<std::string><br>
+collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,<br>
+ Sema::LookupNameKind LookupKind) {<br>
+ std::vector<std::string> Scopes;<br>
+ VisitedContextCollector Collector;<br>
+ Sem.LookupVisibleDecls(S, LookupKind, Collector,<br>
+ /*IncludeGlobalScope=*/false,<br>
+ /*LoadExternal=*/false);<br>
+<br>
+ Scopes.push_back("");<br>
+ for (const auto *Ctx : Collector.takeVisitedContexts()) {<br>
+ if (isa<NamespaceDecl>(Ctx))<br>
+ Scopes.push_back(printNamespaceScope(*Ctx));<br>
+ }<br>
+ return Scopes;<br>
+}<br>
+<br>
class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {<br>
public:<br>
UnresolvedNameRecorder(llvm::Optional<UnresolvedName> &LastUnresolvedName)<br>
@@ -321,48 +341,30 @@ public:<br>
if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))<br>
return clang::TypoCorrection();<br>
<br>
- // This is not done lazily because `SS` can get out of scope and it's<br>
- // relatively cheap.<br>
auto Extracted = extractUnresolvedNameCheaply(<br>
SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,<br>
static_cast<Sema::LookupNameKind>(LookupKind) ==<br>
Sema::LookupNameKind::LookupNestedNameSpecifierName);<br>
if (!Extracted)<br>
return TypoCorrection();<br>
- auto CheapUnresolved = std::move(*Extracted);<br>
+<br>
UnresolvedName Unresolved;<br>
- Unresolved.Name = CheapUnresolved.Name;<br>
+ Unresolved.Name = Extracted->Name;<br>
Unresolved.Loc = Typo.getBeginLoc();<br>
-<br>
- if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.<br>
+ if (!Extracted->ResolvedScope && !S) // Give up if no scope available.<br>
return TypoCorrection();<br>
<br>
- auto *Sem = SemaPtr; // Avoid capturing `this`.<br>
- Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {<br>
- std::vector<std::string> Scopes;<br>
- if (CheapUnresolved.ResolvedScope) {<br>
- Scopes.push_back(*CheapUnresolved.ResolvedScope);<br>
- } else {<br>
- assert(S);<br>
- // No scope specifier is specified. Collect all accessible scopes in the<br>
- // context.<br>
- VisitedContextCollector Collector;<br>
- Sem->LookupVisibleDecls(<br>
- S, static_cast<Sema::LookupNameKind>(LookupKind), Collector,<br>
- /*IncludeGlobalScope=*/false,<br>
- /*LoadExternal=*/false);<br>
-<br>
- Scopes.push_back("");<br>
- for (const auto *Ctx : Collector.takeVisitedContexts())<br>
- if (isa<NamespaceDecl>(Ctx))<br>
- Scopes.push_back(printNamespaceScope(*Ctx));<br>
- }<br>
+ if (Extracted->ResolvedScope)<br>
+ Unresolved.Scopes.push_back(*Extracted->ResolvedScope);<br>
+ else // no qualifier or qualifier is unresolved.<br>
+ Unresolved.Scopes = collectAccessibleScopes(<br>
+ *SemaPtr, Typo, S, static_cast<Sema::LookupNameKind>(LookupKind));<br>
+<br>
+ if (Extracted->UnresolvedScope) {<br>
+ for (std::string &Scope : Unresolved.Scopes)<br>
+ Scope += *Extracted->UnresolvedScope;<br>
+ }<br>
<br>
- if (CheapUnresolved.UnresolvedScope)<br>
- for (auto &Scope : Scopes)<br>
- Scope.append(*CheapUnresolved.UnresolvedScope);<br>
- return Scopes;<br>
- };<br>
LastUnresolvedName = std::move(Unresolved);<br>
<br>
// Never return a valid correction to try to recover. Our suggested fixes<br>
@@ -384,14 +386,13 @@ IncludeFixer::unresolvedNameRecorder() {<br>
std::vector<Fix> IncludeFixer::fixUnresolvedName() const {<br>
assert(LastUnresolvedName.hasValue());<br>
auto &Unresolved = *LastUnresolvedName;<br>
- std::vector<std::string> Scopes = Unresolved.GetScopes();<br>
vlog("Trying to fix unresolved name \"{0}\" in scopes: [{1}]",<br>
- Unresolved.Name, llvm::join(Scopes.begin(), Scopes.end(), ", "));<br>
+ Unresolved.Name, llvm::join(Unresolved.Scopes, ", "));<br>
<br>
FuzzyFindRequest Req;<br>
Req.AnyScope = false;<br>
Req.Query = Unresolved.Name;<br>
- Req.Scopes = Scopes;<br>
+ Req.Scopes = Unresolved.Scopes;<br>
Req.RestrictForCodeCompletion = true;<br>
Req.Limit = 100;<br>
<br>
<br>
Modified: clang-tools-extra/trunk/clangd/IncludeFixer.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.h?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.h?rev=368019&r1=368018&r2=368019&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/IncludeFixer.h (original)<br>
+++ clang-tools-extra/trunk/clangd/IncludeFixer.h Tue Aug 6 04:37:50 2019<br>
@@ -58,9 +58,7 @@ private:<br>
struct UnresolvedName {<br>
std::string Name; // E.g. "X" in foo::X.<br>
SourceLocation Loc; // Start location of the unresolved name.<br>
- // Lazily get the possible scopes of the unresolved name. `Sema` must be<br>
- // alive when this is called.<br>
- std::function<std::vector<std::string>()> GetScopes;<br>
+ std::vector<std::string> Scopes; // Namespace scopes we should search in.<br>
};<br>
<br>
/// Records the last unresolved name seen by Sema.<br>
<br>
Modified: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=368019&r1=368018&r2=368019&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=368019&r1=368018&r2=368019&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp Tue Aug 6 04:37:50 2019<br>
@@ -797,6 +797,27 @@ namespace c {<br>
"Add include \"x.h\" for symbol a::X")))));<br>
}<br>
<br>
+TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {<br>
+ Annotations Test(R"cpp(<br>
+ template <typename T> struct Templ {<br>
+ template <typename U><br>
+ typename U::type operator=(const U &);<br>
+ };<br>
+<br>
+ struct A {<br>
+ Templ<char> s;<br>
+ A() { [[a]]; } // this caused crashes if we compute scopes lazily.<br>
+ };<br>
+ )cpp");<br>
+<br>
+ auto TU = TestTU::withCode(Test.code());<br>
+ auto Index = buildIndexWithSymbol({});<br>
+ TU.ExternalIndex = Index.get();<br>
+<br>
+ EXPECT_THAT(TU.build().getDiagnostics(),<br>
+ ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));<br>
+}<br>
+<br>
TEST(DiagsInHeaders, DiagInsideHeader) {<br>
Annotations Main(R"cpp(<br>
#include [["a.h"]]<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail-m_-2625572906849018657gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>