[clang][Sema] Fix a bug when instantiating a lambda with requires clause #65193

0x59616e · 2023-09-02T01:52:09Z

Instantiating a lambda at a scope different from where it is defined will paralyze clang if the trailing require clause refers to local variables. This patch fixes this by re-adding the local variables to LocalInstantiationScope.

Fixes #64462

cor3ntin · 2023-09-02T05:58:57Z

Just to note that this has some overlap with https://reviews.llvm.org/D159126

cor3ntin · 2023-09-11T13:04:15Z

Could you rebase on top of main? There have been a few changes to lambda that i think address part of the issues here, but this patch is still very much needed.

We add a bug when referring to a capture in a decltype or a require would cause a crash, and that is fixed. I suspect the salient part of your patch is the addition of the parameters of the enclosing function.

llvmbot · 2023-09-12T12:07:22Z

@llvm/pr-subscribers-clang

Changes

Instantiating a lambda at a scope different from where it is defined will paralyze clang if the trailing require clause refers to local variables. This patch fixes this by re-adding the local variables to LocalInstantiationScope.

Fixes #64462

Full diff: https://github.com/llvm/llvm-project/pull/65193.diff

2 Files Affected:

(modified) clang/lib/Sema/SemaConcept.cpp (+61)
(added) clang/test/SemaCXX/pr64462.cpp (+20)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index d1fa8e7831225b7..45a25cdcb88c8d3 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -567,6 +567,59 @@ bool Sema::addInstantiatedCapturesToScope(
   return false;
 }
 
+static void addDeclsFromParentScope(Sema &S, FunctionDecl *FD,
+                                    LocalInstantiationScope &Scope) {
+  assert(isLambdaCallOperator(FD) && "FD must be a lambda call operator");
+
+  LambdaScopeInfo *LSI = cast(S.getFunctionScopes().back());
+
+  auto captureVar = [&](VarDecl *VD) {
+    LSI->addCapture(VD, /*isBlock=*/false, /*isByref=*/false,
+                    /*isNested=*/false, VD->getBeginLoc(), SourceLocation(),
+                    VD->getType(), /*Invalid=*/false);
+  };
+
+  FD = dyn_cast(FD->getParent()->getParent());
+  FunctionDecl *Pattern = nullptr;
+
+  if (!FD || !FD->isTemplateInstantiation())
+    return;
+
+  Pattern = FD->getPrimaryTemplate()->getTemplatedDecl();
+
+  for (unsigned I = 0; I < Pattern->getNumParams(); ++I) {
+    ParmVarDecl *PVD = Pattern->getParamDecl(I);
+    if (!PVD->isParameterPack()) {
+      Scope.InstantiatedLocal(PVD, FD->getParamDecl(I));
+      captureVar(FD->getParamDecl(I));
+      continue;
+    }
+
+    Scope.MakeInstantiatedLocalArgPack(PVD);
+
+    for (ParmVarDecl *Inst : FD->parameters().drop_front(I)) {
+      Scope.InstantiatedLocalPackArg(PVD, Inst);
+      captureVar(Inst);
+    }
+  }
+
+  for (auto *decl : Pattern->decls()) {
+    if (!isa(decl) || isa(decl))
+      continue;
+
+    IdentifierInfo *II = cast(decl)->getIdentifier();
+    auto it = llvm::find_if(FD->decls(), [&](Decl *inst) {
+      VarDecl *VD = dyn_cast(inst);
+      return VD && VD->isLocalVarDecl() && VD->getIdentifier() == II;
+    });
+
+    assert(it != FD->decls().end() && "Cannot find the instantiated variable.");
+
+    Scope.InstantiatedLocal(decl, *it);
+    captureVar(cast(*it));
+  }
+}
+
 bool Sema::SetupConstraintScope(
     FunctionDecl *FD, std::optional> TemplateArgs,
     MultiLevelTemplateArgumentList MLTAL, LocalInstantiationScope &Scope) {
@@ -684,9 +737,11 @@ bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
       CtxToSave = CtxToSave->getNonTransparentContext();
   }
 
+  const bool shouldAddDeclsFromParentScope = !CtxToSave->Encloses(CurContext);
   ContextRAII SavedContext{*this, CtxToSave};
   LocalInstantiationScope Scope(*this, !ForOverloadResolution ||
                                            isLambdaCallOperator(FD));
+
   std::optional MLTAL =
       SetupConstraintCheckingTemplateArgumentsAndScope(
           const_cast(FD), {}, Scope);
@@ -705,6 +760,9 @@ bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
   LambdaScopeForCallOperatorInstantiationRAII LambdaScope(
       *this, const_cast(FD), *MLTAL, Scope);
 
+  if (isLambdaCallOperator(FD) && shouldAddDeclsFromParentScope)
+    addDeclsFromParentScope(*this, const_cast(FD), Scope);
+
   return CheckConstraintSatisfaction(
       FD, {FD->getTrailingRequiresClause()}, *MLTAL,
       SourceRange(UsageLoc.isValid() ? UsageLoc : FD->getLocation()),
@@ -896,6 +954,9 @@ bool Sema::CheckInstantiatedFunctionTemplateConstraints(
   LambdaScopeForCallOperatorInstantiationRAII LambdaScope(
       *this, const_cast(Decl), *MLTAL, Scope);
 
+  if (isLambdaCallOperator(Decl))
+    addDeclsFromParentScope(*this, Decl, Scope);
+
   llvm::SmallVector Converted;
   return CheckConstraintSatisfaction(Template, TemplateAC, Converted, *MLTAL,
                                      PointOfInstantiation, Satisfaction);
diff --git a/clang/test/SemaCXX/pr64462.cpp b/clang/test/SemaCXX/pr64462.cpp
new file mode 100644
index 000000000000000..cc8b5510d1a823a
--- /dev/null
+++ b/clang/test/SemaCXX/pr64462.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+
+auto c1(auto f, auto ...fs) {
+  constexpr bool a = true;
+  // expected-note@+2{{because substituted constraint expression is ill-formed: no matching function for call to 'c1'}}
+  // expected-note@+1{{candidate template ignored: constraints not satisfied [with auto:1 = bool}}
+  return [](auto) requires a && (c1(fs...)) {};
+}
+
+auto c2(auto f, auto ...fs) {
+  constexpr bool a = true;
+  // expected-note@+2{{because substituted constraint expression is ill-formed: no matching function for call to 'c2'}}
+  // expected-note@+1{{candidate function not viable: constraints not satisfied}}
+  return []() requires a && (c2(fs...)) {};
+}
+
+void foo() {
+  c1(true)(true); // expected-error {{no matching function for call to object of type}}
+  c2(true)(); // expected-error {{no matching function for call to object of type}}
+}

cor3ntin · 2023-09-12T13:33:01Z

clang/lib/Sema/SemaConcept.cpp

+  assert(isLambdaCallOperator(FD) && "FD must be a lambda call operator");
+
+  LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(S.getFunctionScopes().back());
+
+  auto captureVar = [&](VarDecl *VD) {
+    LSI->addCapture(VD, /*isBlock=*/false, /*isByref=*/false,
+                    /*isNested=*/false, VD->getBeginLoc(), SourceLocation(),
+                    VD->getType(), /*Invalid=*/false);
+  };
+


That logic is already in LambdaScopeForCallOperatorInstantiationRAII, you need to add it again

I couldn't find where this addCapture logic is in LambdaScopeForCallOperatorInstantiationRAII. Would you mind giving more hints ? Thanks.

clang/lib/Sema/SemaConcept.cpp

cor3ntin

We have addInstantiatedParametersToScope - Which we use in SetupConstraints for example
We could reuse that to add the parameter of the enclosing function.
We could similarly add an addInstantiatedLocalVarsToScope method doing the same thing.

And the logic could be moved to LambdaScopeForCallOperatorInstantiationRAII

@erichkeane for additional feedback

cor3ntin · 2023-09-12T13:46:51Z

clang/lib/Sema/SemaConcept.cpp

+
+    assert(it != FD->decls().end() && "Cannot find the instantiated variable.");
+
+    Scope.InstantiatedLocal(decl, *it);


A I understand, this is the important bit, we don't care they are captures, all captures are added unconditionally

I'm having difficulty understanding the problem. Is there anything incorrect here ? It would be great if more context could be provided. Thanks.

clang/lib/Sema/SemaConcept.cpp

erichkeane · 2023-09-12T14:02:55Z

We have addInstantiatedParametersToScope - Which we use in SetupConstraintsfor example We could reuse that to add the parameter of the enclosing function. We could similarly add anaddInstantiatedLocalVarsToScope` method doing the same thing.

And the logic could be moved to LambdaScopeForCallOperatorInstantiationRAII

@erichkeane for additional feedback

That sounds like a really good idea to me. We seem to be messing with this sort of thing all over the place, so having it all in 1 place seems beneficial.

cor3ntin · 2023-09-12T14:05:55Z

There is also the question of what happen in the deeply nested case

int foo(auto...);
auto f(auto... a) {
   return [] {
           return [] () requires requires { foo(a...) ;} {};
   }();
}

auto g = f()();

we may have to visit all functions recursively from the bottom up to add their locals / parameters / captures

0x59616e · 2023-09-15T09:19:59Z

Support for nested lambda has been added. I will address your kindly feedbacks later.

cor3ntin

I found a couple of typos, and this needs a release note to indicate #64462 was fixed.

I'd like @erichkeane to take a last look but otherwise I think this looks reasonable

(Sorry for the delay)

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

clang/include/clang/Sema/Sema.h

erichkeane

Just a pair of nits based on the style guide, else LGTM.

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaLambda.cpp

0x59616e · 2023-10-03T10:29:56Z

The typo and the style issues have been fixed. Thank you. Now I'm going to rebase to main and add release note.

Instantiating a lambda at a scope different from its definition scope will paralyze clang if the trailing require clause refers to local variables of that definition scope. This patch fixes this by re-adding the local variables to `LocalInstantiationScope`. Fixes llvm#64462

cor3ntin

LGTM, thanks! (Feel free to squash-merge yourself)

0x59616e · 2023-10-04T02:19:00Z

Thank you for your review. I appreciate your feedback ;)

Local branch amd-gfx b60b014 Merged main:9370271ec5de into amd-gfx:a9743de9eb8f Remote branch main 548d67a [clang][Sema] Fix a bug when instantiating a lambda with requires clause (llvm#65193)

RKSimon · 2024-06-21T14:44:41Z

clang/lib/Sema/SemaLambda.cpp


-  SemasRef.RebuildLambdaScopeInfo(cast<CXXMethodDecl>(FD));
+      if (!FD || !Pattern)


@0x59616e Should the !FD be !ParentFD instead? Just noticed this in a static analysis report

I'm looking into it.

…y out logic We should be checking for a failed dyn_cast on the ParentFD result - not the loop invariant FD root value. Seems to have been introduced in llvm#65193 Noticed by static analyser (I have no specific test case).

…y out logic (#96888) We should be checking for a failed dyn_cast on the ParentFD result - not the loop invariant FD root value. Seems to have been introduced in #65193 Noticed by static analyser (I have no specific test case).

…y out logic (llvm#96888) We should be checking for a failed dyn_cast on the ParentFD result - not the loop invariant FD root value. Seems to have been introduced in llvm#65193 Noticed by static analyser (I have no specific test case).

0x59616e requested review from cor3ntin and erichkeane September 2, 2023 01:52

0x59616e requested a review from a team as a code owner September 2, 2023 01:52

0x59616e marked this pull request as draft September 2, 2023 01:59

0x59616e force-pushed the pr64462-2 branch from b63dd30 to a65cb21 Compare September 2, 2023 03:03

0x59616e marked this pull request as ready for review September 2, 2023 03:05

cor3ntin added clang:frontend Language frontend issues, e.g. anything involving "Sema" lambda C++11 lambda expressions labels Sep 2, 2023

cor3ntin mentioned this pull request Sep 2, 2023

"Could not find a matching team" error in pre-commit CI of some github PRs #65207

Closed

0x59616e marked this pull request as draft September 12, 2023 10:29

0x59616e force-pushed the pr64462-2 branch from a65cb21 to 0da3443 Compare September 12, 2023 12:06

llvmbot added the clang Clang issues not falling into any other category label Sep 12, 2023

0x59616e force-pushed the pr64462-2 branch from 0da3443 to 3eafb85 Compare September 12, 2023 12:07

0x59616e marked this pull request as ready for review September 12, 2023 12:09

cor3ntin reviewed Sep 12, 2023

View reviewed changes

clang/lib/Sema/SemaConcept.cpp Outdated Show resolved Hide resolved

cor3ntin reviewed Sep 12, 2023

View reviewed changes

cor3ntin reviewed Oct 2, 2023

View reviewed changes

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Outdated Show resolved Hide resolved

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

erichkeane reviewed Oct 2, 2023

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

clang/lib/Sema/SemaLambda.cpp Outdated Show resolved Hide resolved

0x59616e force-pushed the pr64462-2 branch from b500771 to 7562d12 Compare October 3, 2023 10:28

0x59616e added 3 commits October 3, 2023 18:30

Add support for nested lambda.

e391ab7

Move the implementation into LambdaScopeForCallOperatorInstantiationRAII

7edb6a5

0x59616e added 3 commits October 3, 2023 18:30

Fix typo

22a5a4e

Align the code style.

1eda430

Add description in release note.

6fa1071

0x59616e force-pushed the pr64462-2 branch from 7562d12 to 6fa1071 Compare October 3, 2023 10:35

cor3ntin approved these changes Oct 3, 2023

View reviewed changes

0x59616e merged commit 548d67a into llvm:main Oct 4, 2023

0x59616e deleted the pr64462-2 branch October 4, 2023 02:19

RKSimon reviewed Jun 21, 2024

View reviewed changes

RKSimon mentioned this pull request Jun 27, 2024

[Sema] LambdaScopeForCallOperatorInstantiationRAII - fix typo in early out logic #96888

Merged


		assert(it != FD->decls().end() && "Cannot find the instantiated variable.");

		Scope.InstantiatedLocal(decl, *it);


		SemasRef.RebuildLambdaScopeInfo(cast<CXXMethodDecl>(FD));
		if (!FD \|\| !Pattern)

[clang][Sema] Fix a bug when instantiating a lambda with requires clause #65193

[clang][Sema] Fix a bug when instantiating a lambda with requires clause #65193

Uh oh!

Conversation

0x59616e commented Sep 2, 2023

Uh oh!

cor3ntin commented Sep 2, 2023

Uh oh!

cor3ntin commented Sep 11, 2023

Uh oh!

llvmbot commented Sep 12, 2023

Fixes #64462

Uh oh!

cor3ntin Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

0x59616e Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cor3ntin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

0x59616e Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane commented Sep 12, 2023

Uh oh!

cor3ntin commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x59616e commented Sep 15, 2023

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0x59616e commented Oct 3, 2023

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

0x59616e commented Oct 4, 2023

Uh oh!

RKSimon Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

0x59616e Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0x59616e Sep 15, 2023 •

edited

Loading

cor3ntin left a comment •

edited

Loading

cor3ntin commented Sep 12, 2023 •

edited

Loading