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][Interp] Fix stack peek offset for This ptr #70663

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

tbaederr
Copy link
Contributor

Function::getArgSize() include both the instance and the RVO pointer, so we need to subtract here.

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

llvmbot commented Oct 30, 2023

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Function::getArgSize() include both the instance and the RVO pointer, so we need to subtract here.


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

4 Files Affected:

  • (modified) clang/lib/AST/Interp/ByteCodeExprGen.cpp (+8-20)
  • (modified) clang/lib/AST/Interp/ByteCodeExprGen.h (-1)
  • (modified) clang/lib/AST/Interp/Interp.h (+1-1)
  • (modified) clang/test/AST/Interp/literals.cpp (+8)
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 195af664c13dafc..ccfd35b4171b600 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -2263,13 +2263,17 @@ bool ByteCodeExprGen<Emitter>::VisitCallExpr(const CallExpr *E) {
       }
     } else {
       assert(Initializing);
-      if (!isa<CXXMemberCallExpr>(E)) {
-        if (!this->emitDupPtr(E))
-          return false;
-      }
+      if (!this->emitDupPtr(E))
+        return false;
     }
   }
 
+  // Add the (optional, implicit) This pointer.
+  if (const auto *MC = dyn_cast<CXXMemberCallExpr>(E)) {
+    if (!this->visit(MC->getImplicitObjectArgument()))
+      return false;
+  }
+
   // Put arguments on the stack.
   for (const auto *Arg : E->arguments()) {
     if (!this->visit(Arg))
@@ -2326,22 +2330,6 @@ bool ByteCodeExprGen<Emitter>::VisitCallExpr(const CallExpr *E) {
   return true;
 }
 
-template <class Emitter>
-bool ByteCodeExprGen<Emitter>::VisitCXXMemberCallExpr(
-    const CXXMemberCallExpr *E) {
-  if (Initializing) {
-    // If we're initializing, the current stack top is the pointer to
-    // initialize, so dup that so this call has its own version.
-    if (!this->emitDupPtr(E))
-      return false;
-  }
-
-  if (!this->visit(E->getImplicitObjectArgument()))
-    return false;
-
-  return VisitCallExpr(E);
-}
-
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCXXDefaultInitExpr(
     const CXXDefaultInitExpr *E) {
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 83986d3dd579ed6..7f832d93e3a2fe7 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -68,7 +68,6 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
   bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E);
   bool VisitCallExpr(const CallExpr *E);
   bool VisitBuiltinCallExpr(const CallExpr *E);
-  bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *E);
   bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E);
   bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E);
   bool VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E);
diff --git a/clang/lib/AST/Interp/Interp.h b/clang/lib/AST/Interp/Interp.h
index 7dd415d6e460536..8b75c7564632ffe 100644
--- a/clang/lib/AST/Interp/Interp.h
+++ b/clang/lib/AST/Interp/Interp.h
@@ -1855,7 +1855,7 @@ inline bool CheckGlobalCtor(InterpState &S, CodePtr OpPC) {
 inline bool Call(InterpState &S, CodePtr OpPC, const Function *Func) {
   if (Func->hasThisPointer()) {
     size_t ThisOffset =
-        Func->getArgSize() + (Func->hasRVO() ? primSize(PT_Ptr) : 0);
+        Func->getArgSize() - (Func->hasRVO() ? primSize(PT_Ptr) : 0);
 
     const Pointer &ThisPtr = S.Stk.peek<Pointer>(ThisOffset);
 
diff --git a/clang/test/AST/Interp/literals.cpp b/clang/test/AST/Interp/literals.cpp
index ba24955d14503be..68833ec2dc48adf 100644
--- a/clang/test/AST/Interp/literals.cpp
+++ b/clang/test/AST/Interp/literals.cpp
@@ -1062,6 +1062,14 @@ namespace DiscardExprs {
   }
   static_assert(foo<3>() == 3, "");
 
+  struct ATemp {
+    consteval ATemp ret_a() const { return ATemp{}; }
+  };
+
+  void test() {
+    int k = (ATemp().ret_a(), 0);
+  }
+
 #pragma clang diagnostic pop
 }
 #endif

Function::getArgSize() include both the instance and the RVO pointer, so
we need to subtract here.
@tbaederr
Copy link
Contributor Author

tbaederr commented Nov 8, 2023

Ping

1 similar comment
@tbaederr
Copy link
Contributor Author

Ping

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM

@tbaederr tbaederr merged commit 216dfd5 into llvm:main Nov 14, 2023
3 checks passed
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
`Function::getArgSize()` include both the instance and the RVO pointer,
so we need to subtract here.
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.

None yet

3 participants