[clang][Itanium Mangle] Enable mangling of enclosing class for member… #110503

VitaNuo · 2024-09-30T12:50:44Z

…-like friend function templates only if -fclang-abi-compat>19.

llvmbot · 2024-09-30T12:51:23Z

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-clang

Author: Viktoriia Bakalova (VitaNuo)

Changes

…-like friend function templates only if -fclang-abi-compat>=19.

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

2 Files Affected:

(modified) clang/lib/AST/ItaniumMangle.cpp (+10-1)
(modified) clang/test/CodeGenCXX/mangle-concept.cpp (+4)

diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 117255178eebb4..3d9d1e3f22f832 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -693,7 +693,7 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
     if (VD->isExternC())
       return getASTContext().getTranslationUnitDecl();
 
-  if (const auto *FD = D->getAsFunction()) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     if (FD->isExternC())
       return getASTContext().getTranslationUnitDecl();
     // Member-like constrained friends are mangled as if they were members of
@@ -704,6 +704,15 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
       return D->getLexicalDeclContext()->getRedeclContext();
   }
 
+  if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(D)) {
+    // Member-like constrained friends are mangled as if they were members of
+    // the enclosing class.
+    if (FTD->getTemplatedDecl()->isMemberLikeConstrainedFriend() &&
+        getASTContext().getLangOpts().getClangABICompat() >
+            LangOptions::ClangABI::Ver19)
+      return D->getLexicalDeclContext()->getRedeclContext();
+  }
+
   return DC->getRedeclContext();
 }
 
diff --git a/clang/test/CodeGenCXX/mangle-concept.cpp b/clang/test/CodeGenCXX/mangle-concept.cpp
index 6053511c00a7b5..aa0940c35b237a 100644
--- a/clang/test/CodeGenCXX/mangle-concept.cpp
+++ b/clang/test/CodeGenCXX/mangle-concept.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -verify -std=c++20 -emit-llvm -triple %itanium_abi_triple -o - %s -fclang-abi-compat=latest | FileCheck %s
+// RUN: %clang_cc1 -verify -std=c++20 -emit-llvm -triple %itanium_abi_triple -o - %s -fclang-abi-compat=19 | FileCheck %s --check-prefix=CLANG19
 // RUN: %clang_cc1 -verify -std=c++20 -emit-llvm -triple %itanium_abi_triple -o - %s -fclang-abi-compat=17 | FileCheck %s --check-prefix=CLANG17
 // expected-no-diagnostics
 
@@ -59,18 +60,21 @@ namespace test2 {
     // CLANG17: call {{.*}}@_ZN5test21fEz(
     f(ai);
     // CHECK: call {{.*}}@_ZN5test21AIiEF1gIvEEvzQaa4TrueIT_E4TrueITL0__E(
+    // CLANG19: call {{.*}}@_ZN5test2F1gIvEEvzQaa4TrueIT_E4TrueITL0__E(
     // CLANG17: call {{.*}}@_ZN5test21gIvEEvz(
     g(ai);
     // CHECK: call {{.*}}@_ZN5test21hIvEEvzQ4TrueITL0__E(
     // CLANG17: call {{.*}}@_ZN5test21hIvEEvz(
     h(ai);
     // CHECK: call {{.*}}@_ZN5test21AIiEF1iIvQaa4TrueIT_E4TrueITL0__EEEvz(
+    // CLANG19: call {{.*}}@_ZN5test2F1iIvQaa4TrueIT_E4TrueITL0__EEEvz(
     // CLANG17: call {{.*}}@_ZN5test21iIvEEvz(
     i(ai);
     // CHECK: call {{.*}}@_ZN5test21jIvQ4TrueITL0__EEEvz(
     // CLANG17: call {{.*}}@_ZN5test21jIvEEvz(
     j(ai);
     // CHECK: call {{.*}}@_ZN5test21AIiEF1kITk4TruevQ4TrueIT_EEEvz(
+    // CLANG19: call {{.*}}@_ZN5test2F1kITk4TruevQ4TrueIT_EEEvz(
     // CLANG17: call {{.*}}@_ZN5test21kIvEEvz(
     k(ai);
     // CHECK: call {{.*}}@_ZN5test21lITk4TruevEEvz(

hokein · 2024-09-30T12:59:51Z

Thanks.

Can you update the comment here to mention this case?

And I think it is better to move the release note Mangle friend function ... from the Bug Fixes to C++ Support to ABI Changes in This Version.

VitaNuo · 2024-09-30T13:27:47Z

Thanks.

Can you update the comment here to mention this case?

And I think it is better to move the release note Mangle friend function ... from the Bug Fixes to C++ Support to ABI Changes in This Version.

Thank you, the comments should be addressed now.

hokein

Thank you, this looks good to me. Let’s wait for a bit to see if other reviewers have any additional comments.

usx95

Thanks. Left a comment about adding a FIXME to remove ABI compat checks and figuring out the support for past ABI.

usx95 · 2024-09-30T14:35:33Z

clang/lib/AST/ItaniumMangle.cpp

+  if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(D)) {
+    // Member-like constrained friends are mangled as if they were members of
+    // the enclosing class.
+    if (FTD->getTemplatedDecl()->isMemberLikeConstrainedFriend() &&
+        getASTContext().getLangOpts().getClangABICompat() >
+            LangOptions::ClangABI::Ver19)
+      return D->getLexicalDeclContext()->getRedeclContext();


This looks quite a tech debt. I am not aware of our policy regarding the past ABIs we should be supporting, but there definitely should be a time limit.
We should add a fixme to remove ABI checks after supported time

FIXME: Remove check for Ver17 and use D->getAsFunction() after clang-X FIXME: Remove check for Ver19 after clang-Y

@zygoloid Are you aware of a reasonable clang version (X and Y here) after which we should stop supporting Ver17 ? I can guess maybe 1-3 years ?

We have maintained more burdensome things for ABI compatibility for longer. I don't think we have a policy for removing old ABI things, but some Clang users depend on setting the ABI compat back quite a long way. Establishing a policy seems like a good idea.

zygoloid · 2024-09-30T19:39:02Z

clang/lib/AST/ItaniumMangle.cpp

@@ -693,7 +693,7 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
    if (VD->isExternC())
      return getASTContext().getTranslationUnitDecl();

-  if (const auto *FD = D->getAsFunction()) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {


Maybe we can reduce the duplication a little?

Suggested change

if (const auto *FD = dyn_cast<FunctionDecl>(D)) {

if (const auto *FD =

getASTContext().getLangOpts().getClangABICompat() >

LangOptions::ClangABI::Ver19 ? D->getAsFunction() : dyn_cast<FunctionDecl>(D)) {

Thank you, simplified.

github-actions · 2024-10-02T09:43:24Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-10-02T09:43:25Z

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

hokein · 2024-10-02T10:13:30Z

clang/lib/AST/ItaniumMangle.cpp

-    if (FD->isMemberLikeConstrainedFriend() &&
-        getASTContext().getLangOpts().getClangABICompat() >
-            LangOptions::ClangABI::Ver17)
+    if (FD->isMemberLikeConstrainedFriend())


this looks like a behavior change, we should keep the getClangABICompat() > LangOptions::ClangABI::Ver17 check.

Yes sorry, I might have misinterpreted the comment above.

hokein · 2024-10-02T10:14:11Z

clang/lib/AST/ItaniumMangle.cpp

@@ -693,14 +693,13 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
    if (VD->isExternC())
      return getASTContext().getTranslationUnitDecl();

-  if (const auto *FD = D->getAsFunction()) {
-    if (FD->isExternC())


why removing the isExternC()? this will change the behavior.

It's back, thanks.

…-like friend function templates only if ` -fclang-abi-compat>=19`.

usx95

Thanks. LGTM.

llvm#110503) …-like friend function templates only if ` -fclang-abi-compat>19`.

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

VitaNuo linked an issue Sep 30, 2024 that may be closed by this pull request

[abi] Declaring class of a member-like friend method is not mangled. #110088

Closed

VitaNuo requested review from hokein, usx95 and zygoloid September 30, 2024 12:53

VitaNuo self-assigned this Sep 30, 2024

VitaNuo added ABI Application Binary Interface concepts C++20 concepts libc++abi libc++abi C++ Runtime Library. Not libc++. tools:llvm-cxxfilt labels Sep 30, 2024

VitaNuo force-pushed the fix-abi-compat branch 2 times, most recently from f22108b to 53ef5a1 Compare September 30, 2024 13:22

hokein approved these changes Sep 30, 2024

View reviewed changes

usx95 reviewed Sep 30, 2024

View reviewed changes

zygoloid reviewed Sep 30, 2024

View reviewed changes

VitaNuo force-pushed the fix-abi-compat branch from 53ef5a1 to 27c2b3d Compare October 2, 2024 09:23

VitaNuo requested review from rupprecht, keith, ftynse, makslevental, stellaraccident, aartbik, PeimingLiu, yinying-lisa-li, matthias-springer and antiagainst as code owners October 2, 2024 09:38

Endilll requested review from hanhanW, nicolasvasilache, dcaballe and PeimingLiu October 2, 2024 09:41

VitaNuo force-pushed the fix-abi-compat branch from fb2d704 to 27c2b3d Compare October 2, 2024 09:42

VitaNuo removed request for nikic, keith, antiagainst, stellaraccident, rupprecht, JDevlieghere, ftynse, matthias-springer, banach-space, kuhar, makslevental, hanhanW, nicolasvasilache, dcaballe and PeimingLiu October 2, 2024 09:55

hokein reviewed Oct 2, 2024

View reviewed changes

[clang][Itanium Mangle] Enable mangling of enclosing class for member…

927a40f

…-like friend function templates only if ` -fclang-abi-compat>=19`.

VitaNuo force-pushed the fix-abi-compat branch from 27c2b3d to 927a40f Compare October 2, 2024 13:07

Merge branch 'main' into fix-abi-compat

94ff089

usx95 approved these changes Oct 2, 2024

View reviewed changes

VitaNuo merged commit 91e3fb3 into llvm:main Oct 2, 2024
6 of 8 checks passed

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024

[clang][Itanium Mangle] Enable mangling of enclosing class for member… (

ef2a82b

llvm#110503) …-like friend function templates only if ` -fclang-abi-compat>19`.

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024

[clang][Itanium Mangle] Enable mangling of enclosing class for member… (

4b0d122

llvm#110503) …-like friend function templates only if ` -fclang-abi-compat>19`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][Itanium Mangle] Enable mangling of enclosing class for member… #110503

[clang][Itanium Mangle] Enable mangling of enclosing class for member… #110503

VitaNuo commented Sep 30, 2024 •

edited by usx95

Loading

llvmbot commented Sep 30, 2024 •

edited

Loading

hokein commented Sep 30, 2024

VitaNuo commented Sep 30, 2024

hokein left a comment

usx95 left a comment

usx95 Sep 30, 2024

zygoloid Sep 30, 2024

zygoloid Sep 30, 2024

VitaNuo Oct 2, 2024

github-actions bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

hokein Oct 2, 2024

VitaNuo Oct 2, 2024

hokein Oct 2, 2024

VitaNuo Oct 2, 2024 •

edited

Loading

usx95 left a comment

[clang][Itanium Mangle] Enable mangling of enclosing class for member… #110503

[clang][Itanium Mangle] Enable mangling of enclosing class for member… #110503

Conversation

VitaNuo commented Sep 30, 2024 • edited by usx95 Loading

llvmbot commented Sep 30, 2024 • edited Loading

hokein commented Sep 30, 2024

VitaNuo commented Sep 30, 2024

hokein left a comment

Choose a reason for hiding this comment

usx95 left a comment

Choose a reason for hiding this comment

usx95 Sep 30, 2024

Choose a reason for hiding this comment

zygoloid Sep 30, 2024

Choose a reason for hiding this comment

zygoloid Sep 30, 2024

Choose a reason for hiding this comment

VitaNuo Oct 2, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

hokein Oct 2, 2024

Choose a reason for hiding this comment

VitaNuo Oct 2, 2024

Choose a reason for hiding this comment

hokein Oct 2, 2024

Choose a reason for hiding this comment

VitaNuo Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

usx95 left a comment

Choose a reason for hiding this comment

VitaNuo commented Sep 30, 2024 •

edited by usx95

Loading

llvmbot commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

VitaNuo Oct 2, 2024 •

edited

Loading