Skip to content

Conversation

@llvmbot
Copy link
Member

@llvmbot llvmbot commented Feb 13, 2025

Backport 32c8754

Requested by: @HighCommander4

@llvmbot llvmbot added this to the LLVM 20.X Release milestone Feb 13, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Feb 13, 2025

@zyn0217 What do you think about merging this PR to the release branch?

@llvmbot llvmbot requested a review from zyn0217 February 13, 2025 23:42
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 13, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-clang

Author: None (llvmbot)

Changes

Backport 32c8754

Requested by: @HighCommander4


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

2 Files Affected:

  • (modified) clang/lib/AST/Expr.cpp (+16-5)
  • (modified) clang/test/AST/ast-dump-cxx2b-deducing-this.cpp (+13)
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 06b0491442673..aa7e14329a21b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1645,11 +1645,22 @@ SourceLocation CallExpr::getBeginLoc() const {
   if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(this))
     return OCE->getBeginLoc();
 
-  if (const auto *Method =
-          dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
-      Method && Method->isExplicitObjectMemberFunction()) {
-    assert(getNumArgs() > 0 && getArg(0));
-    return getArg(0)->getBeginLoc();
+  // A non-dependent call to a member function with an explicit object parameter
+  // is modelled with the object expression being the first argument, e.g. in
+  // `o.f(x)`, the callee will be just `f`, and `o` will be the first argument.
+  // Since the first argument is written before the callee, the expression's
+  // begin location should come from the first argument.
+  // This does not apply to dependent calls, which are modelled with `o.f`
+  // being the callee.
+  if (!isTypeDependent()) {
+    if (const auto *Method =
+            dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
+        Method && Method->isExplicitObjectMemberFunction()) {
+      bool HasFirstArg = getNumArgs() > 0 && getArg(0);
+      assert(HasFirstArg);
+      if (HasFirstArg)
+        return getArg(0)->getBeginLoc();
+    }
   }
 
   SourceLocation begin = getCallee()->getBeginLoc();
diff --git a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
index 854d12b4cdba6..abe9d6a5b5bc6 100644
--- a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
+++ b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
@@ -13,3 +13,16 @@ void main() {
   // CHECK-NEXT: | `-DeclRefExpr 0x{{[^ ]*}} <col:13> 'int (S &)' lvalue CXXMethod 0x{{[^ ]*}} 'f' 'int (S &)'
 }
 }
+
+namespace GH1269720 {
+template <typename T>
+struct S {
+  void f(this S&);
+  void g(S s) {
+    s.f();
+  }
+  // CHECK: CallExpr 0x{{[^ ]*}} <line:22:5, col:9> '<dependent type>'
+  // CHECK-NEXT: `-MemberExpr 0x{{[^ ]*}} <col:5, col:7> '<bound member function type>' .f
+  // CHECK-NEXT:   `-DeclRefExpr 0x{{[^ ]*}} <col:5> 'S<T>' lvalue ParmVar 0x{{[^ ]*}} 's' 'S<T>'
+};
+}

… explicit object parameter in CallExpr::getBeginLoc() (llvm#126868)

This fixes a crash where CallExpr::getBeginLoc() tries to access the
first argument of a CallExpr representing a call to a function with
an explicit object parameter, assuming that a first argument exists
because it's the object argument.

This is the case for non-dependent calls, but for dependent calls
the object argument is part of the callee (the semantic analysis
that separates it out has not been performed yet) and so there may
not be a first argument.

Fixes llvm#126720

(cherry picked from commit 32c8754)
@tstellar tstellar merged commit 79da30b into llvm:release/20.x Feb 14, 2025
7 of 10 checks passed
@github-actions
Copy link

@HighCommander4 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@HighCommander4
Copy link
Collaborator

@HighCommander4 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

No release note needed here, as this is a fix for a regression introduced in llvm 20. Since it's also being fixed in llvm 20, no user of a release version will ever experience this crash.

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

Development

Successfully merging this pull request may close these issues.

4 participants