[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash

[MaxEW707]

Fixes #115990.

MSVC mangling got inadvertently broken here, #83997, when it was fixed what decl context a lambda is apart of for uneval contexts.

https://godbolt.org/z/K6jb5v145 for reference.

Given the following code snippet

template <typename T>
concept C = requires(const T& t)
{
    { T::test([](){}) };
};

template<typename T, typename = void>
struct Widget;

template <C T>
struct Widget<T> {};

struct Baz
{
    template<typename F>
    static constexpr decltype(auto) test(F&& f) {}
};

void test()
{
    Widget<Baz> w;
}

Baz::test has a deduced return type which means we must instantiate that template even in an unevaluated context.
The lambda inside the concept is within the decl context of struct Widget<T> {};. So we end up needing to mangle a name of Baz::test<Widget<template-type-0-0>::lambda()>>() since the lambda isn't apart of an instantiated substituted class Widget yet at the point the lambda is instantiated.

Upon template instantation of test we end up asking for the mangled name so we can add this instantiation to CodeGenModule::DefferredDecls since test is now referenced but not yet used.

I think the longer term more correct solution is to key DefferedDecls off of something else than the mangled name to avoid having to mangle names for instantations that are referenced but will never be used since they are only instantiated from an unevaluated context.

As a fix for the regression I just created a custom mangling scheme for this case since MSVC has no comparable naming scheme as such a template will never be emitted into the resulting obj as it will never be used.

Similarly the following case is also broken for our msvc compat, https://godbolt.org/z/evPo6KTfE, since we do not mangle decltype(Expr). The Itanium ABI gets away with this as the entire decltype(Expr) expression must be mangled as is for that ABI.The MSVC ABI never mangles a decltype. Only the final canonical deduced type is mangled into the name so we never need to worry about mangling decltype except in the uneval context edge case.
I want to hold off fixing this case until I can see if I can come up with a better more holistic solution around DefferedDecls first. I'll get a github tracking issue created for it however.

If anyone has a better alternative to solve this let me know. I couldn't come up with one since the template instantation code path for uneval contexts takes the same code path as any template instantation and thus requires the ability to get back a mangled name for said template instantation.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Max Winkler (MaxEW707)

Changes

Fixes #115990.

MSVC mangling got inadvertently broken here, #83997, when it was fixed what decl context a lambda is apart of for uneval contexts.

https://godbolt.org/z/K6jb5v145 for reference.

Given the following code snippet

template <typename T>
concept C = requires(const T& t)
{
    { T::test2([](){}) };
};

template<typename T, typename = void>
struct Widget;

template <C T>
struct Widget<T> {};

struct Baz
{
    template<typename F>
    static constexpr decltype(auto) test2(F&& f) {}
};

void test()
{
    Widget<Baz> w;
}

Baz::Test has a deduced return type which means we must instantiate that template even in an unevaluated context.
The lambda inside the concept is within the decl context of struct Widget<T> {};. So we end up needing to mangle a name of Baz::test2<Widget<template-type-0-0>::lambda()>>() since the lambda isn't apart of an instantiated substituted class Widget yet at the point the lambda is instantiated.

Upon template instantation of test2 we end up asking for the mangled name so we can add this instantiation to CodeGenModule::DefferredDecls since test2 is now referenced but not yet used.

I think the longer term more correct solution is to key DefferedDecls off of something else than the mangled name to avoid having to mangle names for instantations that are referenced but will never be used since they are only instantiated from an unevaluated context.

As a fix for the regression I just created a custom mangling scheme for this case since MSVC has no comparable naming scheme as such a template will never be emitted into the resulting obj as it will never be used.

Similarly the following case is also broken for our msvc compat, https://godbolt.org/z/evPo6KTfE, since we do not mangle decltype(Expr). The Itanium ABI gets away with this as the entire decltype(Expr) expression must be mangled as is for that ABI.The MSVC ABI never mangles a decltype. Only the final canonical deduced type is mangled into the name so we never need to worry about mangling decltype except in the uneval context edge case.

If anyone has a better alternative to solve this let me know. I couldn't come up with one since the template instantation code path for uneval contexts takes the same code path as any template instantation and thus requires the ability to get back a mangled name for said template instantation.

---

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

2 Files Affected:

• (modified) clang/lib/AST/MicrosoftMangle.cpp (+9-1)
• (added) clang/test/AST/ms-uneval-context-crash.cpp (+25)

diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index dbc161347025c0..c177e3b1d01ddf 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -3374,7 +3374,15 @@ void MicrosoftCXXNameMangler::mangleType(const MemberPointerType *T,
 
 void MicrosoftCXXNameMangler::mangleType(const TemplateTypeParmType *T,
                                          Qualifiers, SourceRange Range) {
-  Error(Range.getBegin(), "template type parameter type") << Range;
+  Out << '?';
+
+  llvm::SmallString<64> Name;
+  Name += "<TTPT_";
+  Name += llvm::utostr(T->getDepth());
+  Name += "_";
+  Name += llvm::utostr(T->getIndex());
+  Name += ">";
+  mangleSourceName(Name);
 }
 
 void MicrosoftCXXNameMangler::mangleType(const SubstTemplateTypeParmPackType *T,
diff --git a/clang/test/AST/ms-uneval-context-crash.cpp b/clang/test/AST/ms-uneval-context-crash.cpp
new file mode 100644
index 00000000000000..b2f7e58381da81
--- /dev/null
+++ b/clang/test/AST/ms-uneval-context-crash.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fms-compatibility-version=19.33 -emit-llvm %s -o - -triple=x86_64-windows-msvc | FileCheck %s
+
+template <typename T>
+concept C = requires
+{
+    { T::test([](){}) };
+};
+
+template<typename T>
+struct Widget {};
+
+template <C T>
+struct Widget<T> {};
+
+struct Baz
+{
+    template<typename F>
+    static constexpr decltype(auto) test(F&&) {}
+};
+
+void test()
+{
+    Widget<Baz> w;
+}
+// CHECK: @"?test@@YAXXZ"

[zmodem]

I think doing codegen (-emit-llvm) in clang/test/AST/ is unusual, those tests normally just look at the AST (though there are exceptions). Maybe there is a better place for this?

[zmodem]

I suppose the mangling you're adding doesn't actually show up anywhere since it's only used for a concept? But then why do we need to mangle it in the first place?