[clang] SourceLocIdentKind::Function should not be dependent #94942

jcsxky · 2024-06-10T09:11:15Z

Consider the testcase, return type of construct_at is

DecltypeType 0x55555570d8c0 'decltype(new struct BasicPersistent(declval<struct __alloc_traits>()))' sugar dependent
|-CXXNewExpr 0x55555570d880 'struct BasicPersistent *' Function 0x55555570ace0 'operator new' 'void *(unsigned long)'
| `-CXXConstructExpr 0x55555570d848 'struct BasicPersistent' 'void (BasicPersistent &&, SourceLocation)'
|   |-CallExpr 0x55555570aae8 'BasicPersistent':'struct BasicPersistent' xvalue
|   | `-ImplicitCastExpr 0x55555570aad0 'auto (*)(void) -> decltype(__declval<struct __alloc_traits>(0))' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x55555570aa18 'auto (void) -> decltype(__declval<struct __alloc_traits>(0))' lvalue Function 0x55555570a908 'declval' 'auto (void) -> decltype(__declval<struct __alloc_traits>(0))' (FunctionTemplate 0x5555556d0548 'declval') non_odr_use_unevaluated
|   `-CXXDefaultArgExpr 0x55555570d820 'SourceLocation':'struct SourceLocation' has rewritten init
|     `-CallExpr 0x55555570d7b0 'SourceLocation':'struct SourceLocation'
|       |-ImplicitCastExpr 0x55555570d798 'SourceLocation (*)(const char *)' <FunctionToPointerDecay>
|       | `-DeclRefExpr 0x5555556fe5c8 'SourceLocation (const char *)' lvalue CXXMethod 0x5555556fdb28 'Current' 'SourceLocation (const char *)'
|       |   `-NestedNameSpecifier TypeSpec 'struct SourceLocation'
|       `-CXXDefaultArgExpr 0x55555570d7f8 'const char *' has rewritten init
|         `-SourceLocExpr 0x55555570d7d8 'const char *'
`-PointerType 0x555555704800 'struct BasicPersistent *'
  `-SubstTemplateTypeParmType 0x5555557047d0 'struct BasicPersistent' sugar typename depth 0 index 0 _Tp
    |-FunctionTemplate 0x5555556ee048 'construct_at'
    `-RecordType 0x5555556cfd10 'struct BasicPersistent'
      `-CXXRecord 0x5555556fdc98 'BasicPersistent'

when in the process of instantiation. This type is dependent due to the dependency of SourceLocExpr which leads to the crash(only crash in codegen stage and clang-check -ast-dump is OK). SourceLocExpr is created in a dependent context when transforming the return type of construct_at because its context(Sema.CurContext) is a dependent FunctionDecl. But SourceLocExpr should not be dependent when we finish the transformation. Removing SourceLocIdentKind::Function makes SourceLocExpr independent.
attempt to fix #80210

llvmbot · 2024-06-10T09:11:46Z

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Consider the testcase, return type of construct_at is

DecltypeType 0x55555570d8c0 'decltype(new struct BasicPersistent(declval&lt;struct __alloc_traits&gt;()))' sugar dependent
|-CXXNewExpr 0x55555570d880 'struct BasicPersistent *' Function 0x55555570ace0 'operator new' 'void *(unsigned long)'
| `-CXXConstructExpr 0x55555570d848 'struct BasicPersistent' 'void (BasicPersistent &amp;&amp;, SourceLocation)'
|   |-CallExpr 0x55555570aae8 'BasicPersistent':'struct BasicPersistent' xvalue
|   | `-ImplicitCastExpr 0x55555570aad0 'auto (*)(void) -&gt; decltype(__declval&lt;struct __alloc_traits&gt;(0))' &lt;FunctionToPointerDecay&gt;
|   |   `-DeclRefExpr 0x55555570aa18 'auto (void) -&gt; decltype(__declval&lt;struct __alloc_traits&gt;(0))' lvalue Function 0x55555570a908 'declval' 'auto (void) -&gt; decltype(__declval&lt;struct __alloc_traits&gt;(0))' (FunctionTemplate 0x5555556d0548 'declval') non_odr_use_unevaluated
|   `-CXXDefaultArgExpr 0x55555570d820 'SourceLocation':'struct SourceLocation' has rewritten init
|     `-CallExpr 0x55555570d7b0 'SourceLocation':'struct SourceLocation'
|       |-ImplicitCastExpr 0x55555570d798 'SourceLocation (*)(const char *)' &lt;FunctionToPointerDecay&gt;
|       | `-DeclRefExpr 0x5555556fe5c8 'SourceLocation (const char *)' lvalue CXXMethod 0x5555556fdb28 'Current' 'SourceLocation (const char *)'
|       |   `-NestedNameSpecifier TypeSpec 'struct SourceLocation'
|       `-CXXDefaultArgExpr 0x55555570d7f8 'const char *' has rewritten init
|         `-SourceLocExpr 0x55555570d7d8 'const char *'
`-PointerType 0x555555704800 'struct BasicPersistent *'
  `-SubstTemplateTypeParmType 0x5555557047d0 'struct BasicPersistent' sugar typename depth 0 index 0 _Tp
    |-FunctionTemplate 0x5555556ee048 'construct_at'
    `-RecordType 0x5555556cfd10 'struct BasicPersistent'
      `-CXXRecord 0x5555556fdc98 'BasicPersistent'

when in the process of instantiation. This type is dependent due to the dependency of SourceLocExpr which leads to the crash(only crash in codegen stage and clang-check -ast-dump is OK). SourceLocExpr is created in a dependent context when transforming the return type of construct_at because its context(Sema.CurContext) is a dependent FunctionDecl. But SourceLocExpr should not be dependent when we finish the transformation. Removing SourceLocIdentKind::Function makes SourceLocExpr independent.
attempt to fix #80210

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/AST/Expr.h (-1)
(added) clang/test/CodeGenCXX/PR80210.cpp (+34)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cf1ba02cbc4b2..efe78139de6fa 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -846,6 +846,7 @@ Bug Fixes to C++ Support
 - Fix a crash caused by improper use of ``__array_extent``. (#GH80474)
 - Fixed several bugs in capturing variables within unevaluated contexts. (#GH63845), (#GH67260), (#GH69307),
   (#GH88081), (#GH89496), (#GH90669) and (#GH91633).
+- Fix a crash when SourceLocExpr instantiated in a dependent context. (GH80210).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index f2bf667636dc9..92bd7940bdba9 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -4786,7 +4786,6 @@ class SourceLocExpr final : public Expr {
 
   static bool MayBeDependent(SourceLocIdentKind Kind) {
     switch (Kind) {
-    case SourceLocIdentKind::Function:
     case SourceLocIdentKind::FuncSig:
     case SourceLocIdentKind::SourceLocStruct:
       return true;
diff --git a/clang/test/CodeGenCXX/PR80210.cpp b/clang/test/CodeGenCXX/PR80210.cpp
new file mode 100644
index 0000000000000..fdd3beb99209e
--- /dev/null
+++ b/clang/test/CodeGenCXX/PR80210.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++20 %s -emit-llvm -o /dev/null -verify -triple %itanium_abi_triple
+// RUN: %clang_cc1 -std=c++20 %s -emit-llvm -o /dev/null -verify -triple %ms_abi_triple
+
+// expected-no-diagnostics
+
+struct BasicPersistent;
+template <typename> BasicPersistent &&__declval(int);
+template <typename _Tp> auto declval() -> decltype(__declval<_Tp>(0));
+template <typename _Tp> _Tp forward;
+template <typename _Tp, typename... _Args>
+auto construct_at(_Tp *, _Args...) -> decltype(new _Tp(declval<_Args>()...)) {return 0;}
+template <typename> struct allocator;
+template <typename> struct allocator_traits;
+template <typename _Tp> struct allocator_traits<allocator<_Tp>> {
+  using pointer = _Tp *;
+  template <typename _Up, typename... _Args>
+  static void construct(_Up __p, _Args...) {
+    construct_at(__p, forward<_Args>...);
+  }
+};
+struct __alloc_traits : allocator_traits<allocator<BasicPersistent>> {
+} push_back___x;
+__alloc_traits::pointer _M_impl_0;
+template <typename... _Args> void emplace_back(_Args...) {
+  __alloc_traits::construct(_M_impl_0, forward<_Args>...);
+}
+struct SourceLocation {
+  static SourceLocation Current(const char * = __builtin_FUNCTION());
+};
+struct BasicPersistent {
+  BasicPersistent(BasicPersistent &&,
+                  SourceLocation = SourceLocation::Current());
+};
+void CFXJSE_EngineAddObjectToUpArray() { emplace_back(push_back___x); }

tbaederr · 2024-06-10T10:30:30Z

clang/docs/ReleaseNotes.rst

@@ -846,6 +846,7 @@ Bug Fixes to C++ Support
 - Fix a crash caused by improper use of ``__array_extent``. (#GH80474)
 - Fixed several bugs in capturing variables within unevaluated contexts. (#GH63845), (#GH67260), (#GH69307),
  (#GH88081), (#GH89496), (#GH90669) and (#GH91633).
+- Fix a crash when SourceLocExpr instantiated in a dependent context. (GH80210).


Suggested change

- Fix a crash when SourceLocExpr instantiated in a dependent context. (GH80210).

- Fix a crash when SourceLocExpr instantiated in a dependent context. (#GH80210).

cor3ntin · 2024-06-10T10:46:09Z

SourceLocation needs to be dependent because we want to make the name of the function and any template parameter
refers to the instantiated function rather than that of the function template.
This was changed in #78436

The issue is that SourceLocation then is a dependent expression that does not depend on any parameters and its never re-transformed (hence the crash).

I have so far been unable to find a solution. I wonder if we should attach the template parameters to the source location expression, but that seems a bit involved.

jcsxky · 2024-06-10T12:50:42Z

because we want to make the name of the function and any template parameter
refers to the instantiated function rather than that of the function template.

Does that mean we should transform the SourceLocExpr into a certain function name when instantiating if it is __builtin_FUNCTION?

jcsxky · 2024-06-12T13:15:17Z

SourceLocation needs to be dependent because we want to make the name of the function and any template parameter refers to the instantiated function rather than that of the function template. This was changed in #78436

The issue is that SourceLocation then is a dependent expression that does not depend on any parameters and its never re-transformed (hence the crash).

I have so far been unable to find a solution. I wonder if we should attach the template parameters to the source location expression, but that seems a bit involved.

I tested this patch locally with the code from #78128 and it works well. I doubt whether we need to make SourceLocation dependent when Kind is SourceLocIdentKind::Function. I don't come up with a testcase that make this patch failed or crash.

jcsxky · 2024-06-14T02:44:44Z

struct BasicPersistent;

struct SourceLocation {
  static constexpr const char* Current(const char * func = __builtin_FUNCTION()) {
		return func;
	}
};

template <typename> BasicPersistent &&__declval(int);
template <typename _Tp> auto declval() -> decltype(__declval<_Tp>(0));
template <typename _Tp> _Tp forward;

template <typename _Tp, typename... _Args>
auto construct_at(_Tp *, _Args...) -> decltype(new _Tp(declval<_Args>()...)) {
	constexpr auto *F = SourceLocation::Current();
	static_assert(__builtin_strlen(F) == 12);
	return 0;
}

template <typename> struct allocator;
template <typename> struct allocator_traits;
template <typename _Tp> struct allocator_traits<allocator<_Tp>> {
  using pointer = _Tp *;
  template <typename _Up, typename... _Args>
  static void construct(_Up __p, _Args...) {
    construct_at(__p, forward<_Args>...);
  }
};

struct __alloc_traits : allocator_traits<allocator<BasicPersistent>> {
} push_back___x;

__alloc_traits::pointer _M_impl_0;
template <typename... _Args> void emplace_back(_Args...) {
  __alloc_traits::construct(_M_impl_0, forward<_Args>...);
}

struct BasicPersistent {
  BasicPersistent(BasicPersistent &&,
                  const char* = SourceLocation::Current()) {}
};

void CFXJSE_EngineAddObjectToUpArray() { emplace_back(push_back___x); }

I modify the testcase as above and I think we just need to make dependent if Kind is SourceLocIdentKind::SourceLocStruct. Could you please take another look? @cor3ntin

cor3ntin · 2024-06-14T09:43:25Z

So, looked into this more.
You are correct that Function needs not be dependent because we only ever show the name of the function unqualified.
However, FunctionSig does.
Here is an example of what we are trying to preserve https://godbolt.org/z/M3eTa8nn5 (the clang 17 behavior was incorrect)

I apologize for not realizing earlier that your change was actually sensible in the case of Function

zyn0217 · 2024-06-15T09:39:23Z

nit: Please rephrase the commit message before you merge this patch. A large piece of AST involving memory addresses is really hard to decipher.

In addition, it would be nice if you can simplify the test case further - it's super unclear why a bunch of templates are there outside of the SourceLocation::Current() - probably one or two is sufficient?

jcsxky · 2024-06-16T08:16:58Z

So, looked into this more. You are correct that Function needs not be dependent because we only ever show the name of the function unqualified. However, FunctionSig does. Here is an example of what we are trying to preserve https://godbolt.org/z/M3eTa8nn5 (the clang 17 behavior was incorrect)

I apologize for not realizing earlier that your change was actually sensible in the case of Function

Never mind! After looking into the code a little bit more, FunctionSig indeed needs to be dependent.

template<int N>
constexpr int g() {
  return N;
}

template <typename T>
struct S {
    void f(auto) {
        constexpr const char* F = __builtin_FUNCSIG();
        static_assert(__builtin_strlen(F)==g<__builtin_strlen(F)>());
    }

};

void f(){
	S<int>{}.f(0);
}

However, it still crash when we changed from __builtin_FUNCTION to __builtin_FUNCSIG in the testcase from #80210. The underlying issue is that CurContext is FunctionTemplateDecl which is dependent and we used it as ParentContext of SourceLocExpr when transforming decltype of function construct_at. This makes transformed return type of construct_at dependent which is not what we want. I will close this patch since it doesn't resolve the underlying issue. Thanks for your time for the review! @cor3ntin

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

jcsxky requested review from AaronBallman, cor3ntin and erichkeane June 10, 2024 09:46

tbaederr reviewed Jun 10, 2024

View reviewed changes

[clang] SourceLocIdentKind::Function should not be dependent

96f4e2a

jcsxky force-pushed the fix-80210 branch from 8ad1b50 to 96f4e2a Compare June 13, 2024 13:13

jcsxky closed this Jun 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] SourceLocIdentKind::Function should not be dependent #94942

[clang] SourceLocIdentKind::Function should not be dependent #94942

Uh oh!

jcsxky commented Jun 10, 2024

Uh oh!

llvmbot commented Jun 10, 2024

Uh oh!

tbaederr Jun 10, 2024

Uh oh!

cor3ntin commented Jun 10, 2024

Uh oh!

jcsxky commented Jun 10, 2024

Uh oh!

jcsxky commented Jun 12, 2024

Uh oh!

jcsxky commented Jun 14, 2024

Uh oh!

cor3ntin commented Jun 14, 2024

Uh oh!

zyn0217 commented Jun 15, 2024

Uh oh!

jcsxky commented Jun 16, 2024

Uh oh!

Uh oh!

	- Fix a crash when SourceLocExpr instantiated in a dependent context. (GH80210).
	- Fix a crash when SourceLocExpr instantiated in a dependent context. (#GH80210).

[clang] SourceLocIdentKind::Function should not be dependent #94942

[clang] SourceLocIdentKind::Function should not be dependent #94942

Uh oh!

Conversation

jcsxky commented Jun 10, 2024

Uh oh!

llvmbot commented Jun 10, 2024

Uh oh!

tbaederr Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented Jun 10, 2024

Uh oh!

jcsxky commented Jun 10, 2024

Uh oh!

jcsxky commented Jun 12, 2024

Uh oh!

jcsxky commented Jun 14, 2024

Uh oh!

cor3ntin commented Jun 14, 2024

Uh oh!

zyn0217 commented Jun 15, 2024

Uh oh!

jcsxky commented Jun 16, 2024

Uh oh!

Uh oh!