[clang][AST] Handle dependent representation of call to function with explicit object parameter in CallExpr::getBeginLoc() #126868

HighCommander4 · 2025-02-12T07:40:13Z

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 #126720

llvmbot · 2025-02-12T07:40:45Z

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Fixes #126720

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

2 Files Affected:

(modified) clang/lib/AST/Expr.cpp (+5-2)
(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 c22aa66ba2cfb..55ef3f4748e29 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1648,8 +1648,11 @@ SourceLocation CallExpr::getBeginLoc() const {
   if (const auto *Method =
           dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
       Method && Method->isExplicitObjectMemberFunction()) {
-    assert(getNumArgs() > 0 && getArg(0));
-    return getArg(0)->getBeginLoc();
+    if (!isTypeDependent()) {
+      assert(getNumArgs() > 0 && getArg(0));
+      if (getNumArgs() > 0 && getArg(0))
+        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>'
+};
+}

HighCommander4 · 2025-02-12T07:43:22Z

clang/lib/AST/Expr.cpp

@@ -1648,8 +1648,11 @@ SourceLocation CallExpr::getBeginLoc() const {
  if (const auto *Method =
          dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
      Method && Method->isExplicitObjectMemberFunction()) {
-    assert(getNumArgs() > 0 && getArg(0));
-    return getArg(0)->getBeginLoc();
+    if (!isTypeDependent()) {


My rationale for checking isTypeDependent() on this line is that I traced the difference between the dependent and non-dependent cases discussed in this comment to this check in Sema::BuildCallExpr().

Can we move the isTypeDependent() check up to the point where we check isExplicitObjectMemberFunction(), and add a FIXME explaining why this only applies to non-dependent call expressions?

i.e.

// FIXME: Dependent call expressions are not handled because ... if (!isTypeDependent()) { ... }

Done.

Note, I added a comment, but not a "FIXME", as there isn't anything remaining to fix -- the fall-through behaviour (returning getCallee()->getBeginLoc()) is the correct one for the dependent case, because in the dependent case the callee includes the object argument.

HighCommander4 · 2025-02-12T07:45:24Z

clang/lib/AST/Expr.cpp

-    return getArg(0)->getBeginLoc();
+    if (!isTypeDependent()) {
+      assert(getNumArgs() > 0 && getArg(0));
+      if (getNumArgs() > 0 && getArg(0))


In addition to fixing the reported test case by adding the isTypeDependent() check, I made this branch more defensive in general, by checking the condition getNumArgs() > 0 && getArg(0) after asserting it. That way, if there are additional edge cases that would fail this check, they still trip the assertion, but production builds avoid a crash in favour of a much more benign error (a suboptimal begin location).

I don't think we do this elsewhere and in fact I think we prefer a crash we are violating an invariant and we want a hard fail so we can identify and fix ASAP.

Most folks don't run w/ assertions enabled and we could be hiding a latent bug for a long time.

IMO, the assert by itself is sufficient. Otherwise everyone is paying the overhead for the check which should never fail anyway.

I agree with these arguments against checking an asserted condition in general.

However, I think in this case it might make sense, for two reasons:

My level of confidence that "((callee is an explicit object member function) and (call is not dependent)) implies (call has a first argument)" is in fact an invariant is not very high.

The impact of hiding a bug here is fairly low (slightly inaccurate source range, likely to only affect tools like clangd which perform hit-testing on the AST) compared to a production crash.

I'm happy to remove the check if you feel that's the right tradeoff in spite of these considerations, but I wanted to call them out.

zyn0217

Thanks for the patch. While this may not be the most optimal solution, we can likely go with it as-is to put the fire out.

(I think @cor3ntin is not present right now)

clang/lib/AST/Expr.cpp

zyn0217 · 2025-02-12T08:10:11Z

clang/lib/AST/Expr.cpp

@@ -1648,8 +1648,11 @@ SourceLocation CallExpr::getBeginLoc() const {
  if (const auto *Method =
          dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
      Method && Method->isExplicitObjectMemberFunction()) {
-    assert(getNumArgs() > 0 && getArg(0));
-    return getArg(0)->getBeginLoc();
+    if (!isTypeDependent()) {


Can we move the isTypeDependent() check up to the point where we check isExplicitObjectMemberFunction(), and add a FIXME explaining why this only applies to non-dependent call expressions?

i.e.

// FIXME: Dependent call expressions are not handled because ... if (!isTypeDependent()) { ... }

shafik · 2025-02-12T08:18:17Z

Can we please add more details in the summary about e.g. "This fixes a crash when ... and the fix is ..."

clang/lib/AST/Expr.cpp

… explicit object parameter in CallExpr::getBeginLoc() 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 #126720

HighCommander4 · 2025-02-13T07:34:00Z

While this may not be the most optimal solution, we can likely go with it as-is to put the fire out.

Given this comment (i.e. that the behaviour with this patch is correct for the dependent case as well), is there anything else not optimal about this solution?

HighCommander4 · 2025-02-13T07:34:58Z

Can we please add more details in the summary about e.g. "This fixes a crash when ... and the fix is ..."

Done.

zyn0217 · 2025-02-13T08:00:54Z

While this may not be the most optimal solution, we can likely go with it as-is to put the fire out.

Given this comment (i.e. that the behaviour with this patch is correct for the dependent case as well), is there anything else not optimal about this solution?

I was considering finding a better AST model, but it's more complex than the approach so this is good enough for now :P

llvm-ci · 2025-02-13T23:47:29Z

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building clang at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/8011

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************

… 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

… 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)

… 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

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 12, 2025

HighCommander4 mentioned this pull request Feb 12, 2025

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

Closed

HighCommander4 requested review from cor3ntin and zyn0217 February 12, 2025 07:41

HighCommander4 commented Feb 12, 2025

View reviewed changes

HighCommander4 mentioned this pull request Feb 12, 2025

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

Closed

zyn0217 reviewed Feb 12, 2025

View reviewed changes

HighCommander4 force-pushed the users/HighCommander4/issue-126720 branch from a1c57ce to 9430060 Compare February 13, 2025 07:18

zyn0217 reviewed Feb 13, 2025

View reviewed changes

clang/lib/AST/Expr.cpp Outdated Show resolved Hide resolved

HighCommander4 force-pushed the users/HighCommander4/issue-126720 branch from 9430060 to dc03bb6 Compare February 13, 2025 07:31

zyn0217 approved these changes Feb 13, 2025

View reviewed changes

HighCommander4 merged commit 32c8754 into main Feb 13, 2025
8 checks passed

HighCommander4 deleted the users/HighCommander4/issue-126720 branch February 13, 2025 23:32

HighCommander4 mentioned this pull request Feb 13, 2025

Backport fix for #126720 to the 20.x branch #127146

Closed

HighCommander4 mentioned this pull request Mar 8, 2025

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

Closed

[clang][AST] Handle dependent representation of call to function with explicit object parameter in CallExpr::getBeginLoc() #126868

[clang][AST] Handle dependent representation of call to function with explicit object parameter in CallExpr::getBeginLoc() #126868

Uh oh!

Conversation

HighCommander4 commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 12, 2025

Uh oh!

HighCommander4 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

zyn0217 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

shafik Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

HighCommander4 Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zyn0217 Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

shafik commented Feb 12, 2025

Uh oh!

Uh oh!

HighCommander4 commented Feb 13, 2025

Uh oh!

HighCommander4 commented Feb 13, 2025

Uh oh!

zyn0217 commented Feb 13, 2025

Uh oh!

Uh oh!

llvm-ci commented Feb 13, 2025

Uh oh!

Uh oh!

HighCommander4 commented Feb 12, 2025 •

edited

Loading

HighCommander4 Feb 13, 2025 •

edited

Loading