Skip to content

[clang] mangle placeholder for deduced type as a template-prefix #106335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 29, 2024
Merged

[clang] mangle placeholder for deduced type as a template-prefix #106335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b86ebec
[clang] mangle placeholder for deduced type as a template-prefix
mizvekov Aug 28, 2024
d7dc0b3
fixup! [clang] mangle placeholder for deduced type as a template-prefix
mizvekov Aug 28, 2024
01d6dd3
Merge remote-tracking branch 'origin/main' into users/mizvekov/clang-…
mizvekov Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,8 @@ Bug Fixes to C++ Support
- Fix evaluation of the index of dependent pack indexing expressions/types specifiers (#GH105900)
- Correctly handle subexpressions of an immediate invocation in the presence of implicit casts. (#GH105558)
- Clang now correctly handles direct-list-initialization of a structured bindings from an array. (#GH31813)
- Mangle placeholders for deduced types as a template-prefix, such that mangling
of template template parameters uses the correct production. (#GH106182)
- Fixed an assertion failure when converting vectors to int/float with invalid expressions. (#GH105486)

Bug Fixes to AST Handling
Expand Down
12 changes: 4 additions & 8 deletions clang/lib/AST/ItaniumMangle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4442,14 +4442,10 @@ void CXXNameMangler::mangleType(const DeducedTemplateSpecializationType *T) {
if (!Deduced.isNull())
return mangleType(Deduced);

TemplateDecl *TD = T->getTemplateName().getAsTemplateDecl();
assert(TD && "shouldn't form deduced TST unless we know we have a template");

if (mangleSubstitution(TD))
return;

mangleName(GlobalDecl(TD));
addSubstitution(TD);
TemplateName TN = T->getTemplateName();
assert(TN.getAsTemplateDecl() &&
"shouldn't form deduced TST unless we know we have a template");
mangleType(TN);
}

void CXXNameMangler::mangleType(const AtomicType *T) {
Expand Down
12 changes: 12 additions & 0 deletions clang/test/CodeGenCXX/GH106182.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %clang_cc1 -std=c++20 %s -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s

template <template <class> class S>
void create_unique()
requires (S{0}, true) {}

template <class Fn> struct A {
constexpr A(Fn) {};
};

template void create_unique<A>();
// CHECK: @_Z13create_uniqueI1AEvvQcmtlT_Li0EELb1E(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference compared to GCC
https://compiler-explorer.com/z/1EvTrecvh

_Z13create_uniqueI1AEvvQcmtl1SLi0EELb1E
 => void create_unique<A>() requires T{0}, true

_Z13create_uniqueI1AEvvQcmtlT_Li0EELb1E
 => void create_unique<A>() requires S{0}, true

Which i guess is because of the improper canonicalisation

Given that this changes looks correct we should land it, and because main is crashing, we should backport it.
So, we should remove the changelog entry and instead add a changelog entry to 19x explaining the breaking mangling change.

We probably want to make sure this is on GCC's radar.
How hard would it be to get back the older behavior? If at all possible we might want to consider putting the new mangling under an ABI flag.

@AaronBallman @zygoloid @rjmccall

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this changes looks correct we should land it, and because main is crashing, we should backport it.
So, we should remove the changelog entry and instead add a changelog entry to 19x explaining the breaking mangling change.

I think we've missed the 19.x cycle for this because it involves an ABI break (and final goes out in less than a week, which is not enough bake time for us to land it before 19.0.1): https://llvm.org/docs/HowToReleaseLLVM.html#release-patch-rules

If at all possible we might want to consider putting the new mangling under an ABI flag.

+1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman to clarify the status quo is that this crashes in 19. (and did not crash in 18)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The incorrect canonicalization leads us to miss redeclarations:

https://godbolt.org/z/cqsnqjEhT

The previous patch, combined with this patch, fixes that test case entirely.

Also, the tying of the name of the template parameter with the mangling is problematic, as we can reasonably assume we can rename template parameters without causing an ABI break, but not here.

Moreover, when there are multiple declarations, with different names for the template parameter, which name shall we pick? What about when these names differ across TUs?

Lastly, keeping the previous behavior under a different ABI flag seems problematic, as we would be also tying these bugs to the flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman to clarify the status quo is that this crashes in 19. (and did not crash in 18)

Yup, I knew it was a regression (boo!); if we fixed it a few weeks ago, I think it'd be an easier sell for backporting. My concern is we don't have enough bake time for 19.0.1, and once that goes out we no longer take ABI breaking changes for subsequent releases of 19.x.

Lastly, keeping the previous behavior under a different ABI flag seems problematic, as we would be also tying these bugs to the flag.

That's the usual point to the flag; we start doing the right thing by default, but we leave an escape hatch for people relying on the previous ABI (despite the bugs) because we don't know who is relying on the old behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, here is an idea.
Should we

That seems like the cautious approach that avoid regressions/ICE in 19

As for whether we need to preserve the old behavior of mangling in an ABI flag, I'd like the opinion of more people and of what GCC's plan are. I also think we need to wait for feedback on whether it's impacting folks on the 20 cycle. And if people scream we can try to restore the old behavior. No need to carry bugs if no one rely on them

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a reasonable path forward (presuming that we only need to revert one commit); that revert has to happen Real Soon™ though so we can get it backported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman: I think we've missed the 19.x cycle for this because it involves an ABI break (and final goes out in less than a week [...]

The process does have some leeway for further RCs though if there are many (or critical) regressions, c.f. statement from @tru here:

We have previously inserted a 4th RC when there were a lot of outstanding bugs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#106516

16 changes: 16 additions & 0 deletions clang/test/SemaCXX/GH106182.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
// expected-no-diagnostics

template <class Fn> struct A {
constexpr A(Fn) {};
};

template <template <class> class S>
void create_unique()
requires (S{0}, true);

template <template <class> class S>
void create_unique()
requires (S{0}, true) {}

template void create_unique<A>();
Loading