[clang] Compute accurate begin location for CallExpr with explicit object parameter #117841

HighCommander4 · 2024-11-27T03:44:16Z

The explicit object parameter is written before the callee expression, so the begin location should come from the explicit object parameter.

Fixes #116335

llvmbot · 2024-11-27T03:44:50Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

The explicit object parameter is written before the callee expression, so the begin location should come from the explicit object parameter.

Fixes #116335

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

3 Files Affected:

(modified) clang/lib/AST/Expr.cpp (+7)
(modified) clang/lib/Sema/SemaOverload.cpp (+1-1)
(modified) clang/test/AST/ast-dump-cxx2b-deducing-this.cpp (+2-2)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index a4fb4d5a1f2ec4..8b10a2609c2ab0 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1639,6 +1639,13 @@ SourceLocation CallExpr::getBeginLoc() const {
   if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(this))
     return OCE->getBeginLoc();
 
+  if (const CXXMethodDecl *Method =
+          dyn_cast_or_null<const CXXMethodDecl>(getCalleeDecl());
+      Method && Method->isExplicitObjectMemberFunction()) {
+    assert(getNumArgs() > 0 && getArg(0));
+    return getArg(0)->getBeginLoc();
+  }
+
   SourceLocation begin = getCallee()->getBeginLoc();
   if (begin.isInvalid() && getNumArgs() > 0 && getArg(0))
     begin = getArg(0)->getBeginLoc();
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4c9e37bd286dee..e4bf9aa521224b 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -15565,7 +15565,7 @@ ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
     // Build the actual expression node.
     ExprResult FnExpr =
         CreateFunctionRefExpr(*this, Method, FoundDecl, MemExpr,
-                              HadMultipleCandidates, MemExpr->getBeginLoc());
+                              HadMultipleCandidates, MemExpr->getExprLoc());
     if (FnExpr.isInvalid())
       return ExprError();
 
diff --git a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
index 04cff07376885a..1b385e0fc33319 100644
--- a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
+++ b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
@@ -9,7 +9,7 @@ int main() {
   S s;
   int x = s.f();
   // CHECK: CallExpr 0x{{[^ ]*}} <col:11, col:15> 'int
-  // CHECK-NEXT: |-ImplicitCastExpr 0x{{[^ ]*}} <col:11> 'int (*)(S &)' <FunctionToPointerDecay>
-  // CHECK-NEXT: | `-DeclRefExpr 0x{{[^ ]*}} <col:11> 'int (S &)' lvalue CXXMethod 0x{{[^ ]*}} 'f' 'int (S &)'
+  // CHECK-NEXT: |-ImplicitCastExpr 0x{{[^ ]*}} <col:13> 'int (*)(S &)' <FunctionToPointerDecay>
+  // CHECK-NEXT: | `-DeclRefExpr 0x{{[^ ]*}} <col:13> 'int (S &)' lvalue CXXMethod 0x{{[^ ]*}} 'f' 'int (S &)'
 }
 }

zwuis · 2024-11-27T04:03:02Z

clang/lib/AST/Expr.cpp

@@ -1639,6 +1639,13 @@ SourceLocation CallExpr::getBeginLoc() const {
  if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(this))
    return OCE->getBeginLoc();

+  if (const CXXMethodDecl *Method =
+          dyn_cast_or_null<const CXXMethodDecl>(getCalleeDecl());


Suggested change

dyn_cast_or_null<const CXXMethodDecl>(getCalleeDecl());

dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());

zyn0217

I think this also needs a selection tree test to demonstrate the original case works with the current approach.

Besides, If we can't highlight the loci differences (compared to the behavior before the last PR) using simple -ast-dump flags, then perhaps we should reconsider the necessity of clang/test/AST/ast-dump-cxx2b-deducing-this.cpp - removing it might be a good option.

That said, I was still thinking if there's any possibility for us to find a better AST model for such use cases, rather than relying on special-casing it in CallExpr::getBeginLoc

HighCommander4 · 2024-11-27T07:32:15Z

That said, I was still thinking if there's any possibility for us to find a better AST model for such use cases, rather than relying on special-casing it in CallExpr::getBeginLoc

A more general implementation of CallExpr::getBeginLoc that occurs to me is: return the earlier of the callee's begin loc and the first argument's (if present) begin loc.

tbaederr · 2024-11-27T07:42:43Z

clang/lib/AST/Expr.cpp

@@ -1639,6 +1639,13 @@ SourceLocation CallExpr::getBeginLoc() const {
  if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(this))
    return OCE->getBeginLoc();

+  if (const CXXMethodDecl *Method =


Suggested change

if (const CXXMethodDecl *Method =

if (const auto *Method =

HighCommander4 · 2024-11-29T05:06:48Z

Updated to add a clangd test that fails with the previous fix approach.

A more general implementation of CallExpr::getBeginLoc that occurs to me is: return the earlier of the callee's begin loc and the first argument's (if present) begin loc.

I did not do this because accurately determining which of two SourceLocations is earlier requires calling SourceManager::isBeforeInTranslationUnit(), and a SourceManager does not seem to be available in CallExpr::getBeginLoc().

github-actions · 2024-11-29T05:08:27Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ject parameter The explicit object parameter is written before the callee expression, so the begin location should come from the explicit object parameter.

HighCommander4 · 2024-11-29T05:09:43Z

(Removed extraneous formatting changes)

zyn0217

Thanks, while I haven’t come up with a better solution to model this situation, I think this looks good for now.

Please give @cor3ntin some time before merging, in case there are some better approaches that I might have overlooked.

zyn0217 · 2024-11-29T05:40:15Z

clang/lib/AST/Expr.cpp

@@ -1639,11 +1639,19 @@ 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());


nit: I presume the const in <const CXXMethodDecl> isn't necessary, right?

Maybe you could do something like

unsigned Offset = 0; if(isExplicitObjectMemberFunction()) Offset = 1;

To avoid some duplication

Sorry, I'm not quite following this suggestion. What am I supposed to do with Offset?

heh I think there is some opportunities to merge the slightly different if statements (for the explicit and not explicit cases) but it would be a minor improvement so I'll will approve that

heh I think there is some opportunities to merge the slightly different if statements (for the explicit and not explicit cases) but it would be a minor improvement so I'll will approve that

Ah, I see.

I considered doing something like this:

bool UseFirstArgLoc = false; if (/* is explicit object */) UseFirstArgLoc = true; SourceLocation begin = getCallee()->getBeginLoc(); if ((begin.isInvalid() || UseFirstArgLoc) && getNumArgs() > 0 && getArg(0)) begin = getArg(0)->getBeginLoc(); return begin;

But this needlessly evaluates getCallee()->getBeginLoc() in the explicit case, so I opted not to do this.

nikic · 2024-12-06T13:36:04Z

It looks like this causes a significant compile-time regression: https://llvm-compile-time-tracker.com/compare.php?from=2b855dd97092e2178ac5c470a804a17ec440d7e5&to=9ccde12f5eeb91152900082a2ae839e2a9702b31&stat=instructions:u (Maybe most clearly seen during clang bootstrap, where this adds 0.5% to many compilations: https://llvm-compile-time-tracker.com/compare_clang.php?from=2b855dd97092e2178ac5c470a804a17ec440d7e5&to=9ccde12f5eeb91152900082a2ae839e2a9702b31&stat=instructions%3Au&sortBy=interestingness)

Is that expected?

HighCommander4 · 2024-12-08T00:41:59Z

It looks like this causes a significant compile-time regression: https://llvm-compile-time-tracker.com/compare.php?from=2b855dd97092e2178ac5c470a804a17ec440d7e5&to=9ccde12f5eeb91152900082a2ae839e2a9702b31&stat=instructions:u (Maybe most clearly seen during clang bootstrap, where this adds 0.5% to many compilations: https://llvm-compile-time-tracker.com/compare_clang.php?from=2b855dd97092e2178ac5c470a804a17ec440d7e5&to=9ccde12f5eeb91152900082a2ae839e2a9702b31&stat=instructions%3Au&sortBy=interestingness)

Is that expected?

I think it's at least conceivable, as the patch adds an extra branch to CallExpr::getBeginLoc(), which is likely to be called pretty frequently.

The branch is not taken most of the time (it's only taken for calls to functions with an explicit object argument, which is a C++23 feature), so perhaps annotating the branch as "unlikely" is sufficient to avoid the performance regression?

HighCommander4 · 2024-12-08T00:49:30Z

The branch is not taken most of the time (it's only taken for calls to functions with an explicit object argument, which is a C++23 feature), so perhaps annotating the branch as "unlikely" is sufficient to avoid the performance regression?

Ah, no, that's not sufficient because it takes some work (a call to getCalleeDecl()) to compute the branch condition.

We could consider revising the implementation approach to optimize this better; for example, add a "uses explicit object argument" bit to CallExprBits?

tbaederr · 2024-12-08T07:30:13Z

I didn't check if any tests fail but here's a version where CallExpr saves its BeginLoc explicitly: http://llvm-compile-time-tracker.com/compare.php?from=416e4cd332c7421b187844ac9aaf6fe28b575a7d&to=0b6e36fe460409aa59958b79766b4f64a31c97e6&stat=instructions:u

HighCommander4 · 2024-12-08T08:04:34Z

I didn't check if any tests fail but here's a version where CallExpr saves its BeginLoc explicitly: http://llvm-compile-time-tracker.com/compare.php?from=416e4cd332c7421b187844ac9aaf6fe28b575a7d&to=0b6e36fe460409aa59958b79766b4f64a31c97e6&stat=instructions:u

Yeah, this is another alternative, though I wonder if the memory tradeoff it makes (increasing the size of CallExpr by 4 bytes) is worth it.

…licit object parameter (llvm#117841)" This reverts commit 9ccde12.

shafik · 2025-03-07T17:09:10Z

This PR was flagged as the cause of this crash: #130272

HighCommander4 requested a review from zyn0217 November 27, 2024 03:44

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 27, 2024

HighCommander4 mentioned this pull request Nov 27, 2024

CallExpr has incorrect begin location with "deducing this" #116928

Closed

zwuis reviewed Nov 27, 2024

View reviewed changes

zyn0217 reviewed Nov 27, 2024

View reviewed changes

zyn0217 requested a review from cor3ntin November 27, 2024 07:23

tbaederr reviewed Nov 27, 2024

View reviewed changes

HighCommander4 force-pushed the llvm-issue-116335-review branch from d5ef6d5 to 367a0cc Compare November 29, 2024 05:05

llvmbot added clang-tools-extra clangd labels Nov 29, 2024

HighCommander4 requested a review from zyn0217 November 29, 2024 05:06

[clang] Compute accurate begin location for CallExpr with explicit ob…

3e5e55f

…ject parameter The explicit object parameter is written before the callee expression, so the begin location should come from the explicit object parameter.

HighCommander4 force-pushed the llvm-issue-116335-review branch from 367a0cc to 3e5e55f Compare November 29, 2024 05:09

zyn0217 approved these changes Nov 29, 2024

View reviewed changes

cor3ntin approved these changes Dec 5, 2024

View reviewed changes

HighCommander4 merged commit 9ccde12 into llvm:main Dec 6, 2024
8 checks passed

HighCommander4 mentioned this pull request Dec 6, 2024

[clangd] Incorrect navigation caused by deducing this #116335

Closed

zyn0217 mentioned this pull request Feb 11, 2025

[Clang] Frontend crash compiling struct/class method with explicit object parameters #126720

Closed

mordante modified the milestone: LLVM 20.X Release Feb 11, 2025

R-Goc added a commit to R-Goc/llvm-project that referenced this pull request Feb 11, 2025

Revert "[clang] Compute accurate begin location for CallExpr with exp…

f09de04

…licit object parameter (llvm#117841)" This reverts commit 9ccde12.

R-Goc mentioned this pull request Feb 11, 2025

Revert "[clang] Compute accurate begin location for CallExpr with exp… #126789

Closed

jiixyj mentioned this pull request Mar 7, 2025

[clang] Assertion hit in CallExpr::getBeginLoc(): assert(HasFirstArg) fails #130272

Closed

	dyn_cast_or_null<const CXXMethodDecl>(getCalleeDecl());
	dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());

[clang] Compute accurate begin location for CallExpr with explicit object parameter #117841

[clang] Compute accurate begin location for CallExpr with explicit object parameter #117841

Uh oh!

Conversation

HighCommander4 commented Nov 27, 2024

Uh oh!

llvmbot commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwuis Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

HighCommander4 commented Nov 27, 2024

Uh oh!

tbaederr Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 commented Nov 29, 2024

Uh oh!

github-actions bot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HighCommander4 commented Nov 29, 2024

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

zyn0217 Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Dec 6, 2024

Uh oh!

HighCommander4 commented Dec 8, 2024

Uh oh!

HighCommander4 commented Dec 8, 2024

Uh oh!

tbaederr commented Dec 8, 2024

Uh oh!

HighCommander4 commented Dec 8, 2024

Uh oh!

shafik commented Mar 7, 2025

Uh oh!

Uh oh!

llvmbot commented Nov 27, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading