Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Fix the order of addInstantiatedParameters in LambdaScopeForCallOperatorInstantiationRAII #97215

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

LYP951018
Copy link
Contributor

@LYP951018 LYP951018 commented Jun 30, 2024

Currently, addInstantiatedParameters is called from the innermost lambda outward. However, when the function parameters of an inner lambda depend on the function parameters of an outer lambda, it can lead to a crash due to the inability to find a mapping for the instantiated decl.

This PR corrects this behavior by calling addInstantiatedParameters from the outside in.

repro code: https://godbolt.org/z/KbsxWesW6

namespace dependent_param_concept {
template <typename... Ts> void sink(Ts...) {}
void dependent_param() {
  auto L = [](auto... x) {
    return [](decltype(x)... y) { // `y` depends on `x`
      return [](int z)
        requires requires { sink(y..., z); }
      {};
    };
  };
  L(0, 1)(1, 2)(1);
}
} // namespace dependent_param_concept

This PR is a prerequisite for implmenting #61426

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 30, 2024

@llvm/pr-subscribers-clang

Author: Yupei Liu (LYP951018)

Changes

Currently, addInstantiatedParameters is called from the innermost lambda outward. However, when the function parameters of an inner lambda depend on the function parameters of an outer lambda, it can lead to a crash due to the inability to find a mapping for the instantiated decl.

This PR corrects this behavior by calling addInstantiatedParameters from the outside in.

repro code: https://godbolt.org/z/KbsxWesW6

namespace dependent_param_concept {
template &lt;typename... Ts&gt; void sink(Ts...) {}
void dependent_param() {
  auto L = [](auto... x) {
    return [](decltype(x)... y) { // `y` depends on `x`
      return [](int z)
        requires requires { sink(y..., z); }
      {};
    };
  };
  L(0, 1)(1, 2)(1);
}
} // namespace dependent_param_concept

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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaLambda.cpp (+23-14)
  • (modified) clang/test/SemaTemplate/concepts-lambda.cpp (+14)
diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp
index e9476a0c93c5d..b6344f53861f4 100644
--- a/clang/lib/Sema/SemaLambda.cpp
+++ b/clang/lib/Sema/SemaLambda.cpp
@@ -2379,23 +2379,32 @@ Sema::LambdaScopeForCallOperatorInstantiationRAII::
 
   SemaRef.RebuildLambdaScopeInfo(cast<CXXMethodDecl>(FD));
 
-  FunctionDecl *Pattern = getPatternFunctionDecl(FD);
-  if (Pattern) {
-    SemaRef.addInstantiatedCapturesToScope(FD, Pattern, Scope, MLTAL);
+  FunctionDecl *FDPattern = getPatternFunctionDecl(FD);
+  if (!FDPattern)
+    return;
 
-    FunctionDecl *ParentFD = FD;
-    while (ShouldAddDeclsFromParentScope) {
+  SemaRef.addInstantiatedCapturesToScope(FD, FDPattern, Scope, MLTAL);
 
-      ParentFD =
-          dyn_cast<FunctionDecl>(getLambdaAwareParentOfDeclContext(ParentFD));
-      Pattern =
-          dyn_cast<FunctionDecl>(getLambdaAwareParentOfDeclContext(Pattern));
+  if (!ShouldAddDeclsFromParentScope)
+    return;
 
-      if (!FD || !Pattern)
-        break;
+  llvm::SmallVector<std::pair<FunctionDecl *, FunctionDecl *>, 4>
+      ParentInstantiations;
+  std::pair<FunctionDecl *, FunctionDecl *> Current = {FDPattern, FD};
+  while (true) {
+    Current.first = dyn_cast<FunctionDecl>(
+        getLambdaAwareParentOfDeclContext(Current.first));
+    Current.second = dyn_cast<FunctionDecl>(
+        getLambdaAwareParentOfDeclContext(Current.second));
 
-      SemaRef.addInstantiatedParametersToScope(ParentFD, Pattern, Scope, MLTAL);
-      SemaRef.addInstantiatedLocalVarsToScope(ParentFD, Pattern, Scope);
-    }
+    if (!Current.first || !Current.second)
+      break;
+
+    ParentInstantiations.push_back(Current);
+  }
+
+  for (const auto &[Pattern, Inst] : llvm::reverse(ParentInstantiations)) {
+    SemaRef.addInstantiatedParametersToScope(Inst, Pattern, Scope, MLTAL);
+    SemaRef.addInstantiatedLocalVarsToScope(Inst, Pattern, Scope);
   }
 }
diff --git a/clang/test/SemaTemplate/concepts-lambda.cpp b/clang/test/SemaTemplate/concepts-lambda.cpp
index 280be71284f97..252ef08549a48 100644
--- a/clang/test/SemaTemplate/concepts-lambda.cpp
+++ b/clang/test/SemaTemplate/concepts-lambda.cpp
@@ -237,3 +237,17 @@ concept D = []<C T = int>() { return true; }();
 D auto x = 0;
 
 } // namespace GH93821
+
+namespace dependent_param_concept {
+template <typename... Ts> void sink(Ts...) {}
+void dependent_param() {
+  auto L = [](auto... x) {
+    return [](decltype(x)... y) {
+      return [](int z)
+        requires requires { sink(y..., z); }
+      {};
+    };
+  };
+  L(0, 1)(1, 2)(1);
+}
+} // namespace dependent_param_concept

@LYP951018 LYP951018 force-pushed the reverse_instantiated_order branch from 015088d to a997ae7 Compare June 30, 2024 13:21
@LYP951018 LYP951018 closed this Jun 30, 2024
@LYP951018 LYP951018 reopened this Jun 30, 2024
@LYP951018
Copy link
Contributor Author

Sorry for disturbing... I'm trying to get the CI working. I can't figure out why buildkite/github-pull-requests failed...

@LYP951018 LYP951018 force-pushed the reverse_instantiated_order branch from 4174914 to a997ae7 Compare June 30, 2024 15:33
@zyn0217 zyn0217 requested a review from 0x59616e July 2, 2024 14:09
Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think the approach makes sense. Just some nits from me
@cor3ntin do you have any other comments? I think you can do the last approval. :)

Comment on lines -2382 to 2385
FunctionDecl *Pattern = getPatternFunctionDecl(FD);
if (Pattern) {
SemaRef.addInstantiatedCapturesToScope(FD, Pattern, Scope, MLTAL);
FunctionDecl *FDPattern = getPatternFunctionDecl(FD);
if (!FDPattern)
return;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you revert unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep the current change. Firstly, this change is minimal and won’t significantly impact the review workload for this PR. Secondly, it reduces one level of indentation, which slightly improves readability. ;)

clang/lib/Sema/SemaLambda.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaLambda.cpp Outdated Show resolved Hide resolved
@zyn0217
Copy link
Contributor

zyn0217 commented Jul 6, 2024

Oh, I forgot it: this also needs a release note.

@LYP951018 LYP951018 requested a review from zyn0217 July 6, 2024 18:35
Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LYP951018 LYP951018 merged commit f0c7505 into llvm:main Jul 9, 2024
8 checks passed
@LYP951018
Copy link
Contributor Author

Thanks for the review ;)

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…allOperatorInstantiationRAII (llvm#97215)

Currently, `addInstantiatedParameters` is called from the innermost
lambda outward. However, when the function parameters of an inner lambda
depend on the function parameters of an outer lambda, it can lead to a
crash due to the inability to find a mapping for the instantiated decl.

This PR corrects this behavior by calling `addInstantiatedParameters`
from the outside in.

repro code: https://godbolt.org/z/KbsxWesW6

```cpp
namespace dependent_param_concept {
template <typename... Ts> void sink(Ts...) {}
void dependent_param() {
  auto L = [](auto... x) {
    return [](decltype(x)... y) { // `y` depends on `x`
      return [](int z)
        requires requires { sink(y..., z); }
      {};
    };
  };
  L(0, 1)(1, 2)(1);
}
} // namespace dependent_param_concept
```

This PR is a prerequisite for implmenting llvm#61426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants