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 constant evaluating a captured variable in a lambda #68090

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

cor3ntin
Copy link
Contributor

@cor3ntin cor3ntin commented Oct 3, 2023

with an explicit parameter.

We tried to read a pointer to a non-existent This APValue when constant-evaluating an explicit object lambda call operator (the this pointer is never set in explicit object member functions)

Fixes #68070

@cor3ntin cor3ntin added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 3, 2023
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 3, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 3, 2023

@llvm/pr-subscribers-clang

Changes

with an explicit parameter.

Fixes #68070


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

2 Files Affected:

  • (modified) clang/lib/AST/ExprConstant.cpp (+7-1)
  • (modified) clang/test/SemaCXX/cxx2b-deducing-this-constexpr.cpp (+18)
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index a142ea7c47a4730..e8bf037c879c640 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -8366,8 +8366,14 @@ bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
       return false;
 
     if (auto *FD = Info.CurrentCall->LambdaCaptureFields.lookup(VD)) {
+      auto *MD = cast<CXXMethodDecl>(Info.CurrentCall->Callee);
       // Start with 'Result' referring to the complete closure object...
-      Result = *Info.CurrentCall->This;
+      if (MD->isExplicitObjectMemberFunction()) {
+        APValue *RefValue =
+            Info.getParamSlot(Info.CurrentCall->Arguments, MD->getParamDecl(0));
+        Result.setFrom(Info.Ctx, *RefValue);
+      } else
+        Result = *Info.CurrentCall->This;
       // ... then update it to refer to the field of the closure object
       // that represents the capture.
       if (!HandleLValueMember(Info, E, Result, FD))
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this-constexpr.cpp b/clang/test/SemaCXX/cxx2b-deducing-this-constexpr.cpp
index 44de0d711674ba8..9dbea17dd2cae34 100644
--- a/clang/test/SemaCXX/cxx2b-deducing-this-constexpr.cpp
+++ b/clang/test/SemaCXX/cxx2b-deducing-this-constexpr.cpp
@@ -54,3 +54,21 @@ consteval void test() {
     static_assert(*s == 42);
     static_assert((s << 11) == 31);
 }
+
+namespace GH68070 {
+
+constexpr auto f = [x = 3]<typename Self>(this Self&& self) {
+    return x;
+};
+
+auto g = [x = 3]<typename Self>(this Self&& self) {
+    return x;
+};
+
+int test() {
+  constexpr int a = f();
+  static_assert(a == 3);
+  return f() + g();
+}
+
+}

// Start with 'Result' referring to the complete closure object...
Result = *Info.CurrentCall->This;
if (MD->isExplicitObjectMemberFunction()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (MD->isExplicitObjectMemberFunction()) {
if (const auto *MD = cast<CXXMethodDecl>(Info.CurrentCall->Callee);
MD && MD->isExplicitObjectMemberFunction()) {

Does that work?

Info.getParamSlot(Info.CurrentCall->Arguments, MD->getParamDecl(0));
Result.setFrom(Info.Ctx, *RefValue);
} else
Result = *Info.CurrentCall->This;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the CurrentCall->this be set to the first argument for explicit member functions, somewhere where the stack frame is created?

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'd rather not pretend that there is a this object even though it would not correspond to an actual this pointer (and evaluating this is ill formed. I imagine it might make things more difficult to maintain in the future

@shafik
Copy link
Collaborator

shafik commented Oct 3, 2023

Can you write a more detailed description explaining what the problem is what the fix is.

This is what usually ends up in the git log and we want that to be as descriptive as possible for folks who use it to understand changes quickly without digging into details.

MD->isExplicitObjectMemberFunction()) {
APValue *RefValue =
Info.getParamSlot(Info.CurrentCall->Arguments, MD->getParamDecl(0));
Result.setFrom(Info.Ctx, *RefValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So what is the difference between calling setFrom and assigning to Result using assignment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LValue has no assignment operator from APValue (because sometimes you need an ASTContext)

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Probably needs a release note.

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved
Comment on lines +8375 to +8376
} else
Result = *Info.CurrentCall->This;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think LLVM code style suggests curleys in case other branch had them

Suggested change
} else
Result = *Info.CurrentCall->This;
} else {
Result = *Info.CurrentCall->This;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix, thanks!

@cor3ntin
Copy link
Contributor Author

cor3ntin commented Oct 4, 2023

Probably needs a release note.

Nah, this fixes a bug with deducing this I introduced on Monday :)

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

LGTM

@cor3ntin cor3ntin merged commit 49666ec into llvm:main Oct 5, 2023
3 checks passed
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.

ICE in forward from deduced this in lambda
5 participants