[clang] adapt c++17 behavior for dependent decltype-specifiers

[hokein]

Summary

Context: https://reviews.llvm.org/D86048#2254607

[sammccall]

We should check if the mangling matches GCC (or if it depends on -std), if we can.

clang/lib/AST/Type.cpp

3429

isn't this redudant with toDependence(E->getDependence)?

clang/test/SemaCXX/invalid-template-base-specifier.cpp

16

nice!

[sammccall]

Ugh, the hole goes deeper, because the Itanium ABI unambiguously follows the C++11 (instantiation-dependent) behavior.
Definitely need Richard's input here as I'm fairly sure this will break things as-is.

clang/test/CodeGenCXX/mangle-exprs.cpp

215

Amusingly, both GCC and MSVC reject this valid code.

GCC says: "sorry, unimplemented: mangling noexcept_expr"

MSVC says: "'void test5::a(bool)': no function template defined that matches forced instantiation"
MSVC seems to require the expression to be exactly the same as that in the template declaration in order to compile (even when that's not possible)...

clang/test/CodeGenCXX/mangle.cpp

805

GCC mangles this as _ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE, so we're breaking compat here :-\

And in fact we're just incorrect. https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:

If the operand expression of decltype is not instantiation-dependent then the resulting type is encoded directly.

Here, instantiation-dependent *is* explicitly defined as "type-dependent or value-dependent or ...". And therefore these cases must not be "encoded directly", including the motivating case (decltype(N) where N is a typed constant whose initializer is value-dependent).

We could consider not two but *three* types of decltypetypes:

• decltype(type-dependent), which is sugar for a canonical DependentDeclTypeType
• decltype(instantiation-but-not-type-dependent), which is still sugar for a concrete canonical type (per C++17) but mangles using the expr (per cxxabi)
• decltype(non-dependent), which is sugar for a concrete canonical type and mangles directly

This only works if it's OK for mangling to depend on non-canonical type details - I don't know whether this is the case. @rsmith - any hints here?

clang/test/SemaTemplate/dependent-expr.cpp

132

Took me a while to understand this, I think this is an improvement as we now diagnose when we see the template rather than when we see the instantiation.

[sammccall]

clang/test/CodeGenCXX/mangle.cpp

805

Hmm, my naive reading of the mangle code matches what I described.

The big comment in ItaniumMangle talks about related issues: https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572

I don't really understand what's going on here, sorry.

[rsmith]

clang/test/CodeGenCXX/mangle-exprs.cpp

218

As with the other case, the expression here is instantiation-dependent so should be mangled.

clang/test/CodeGenCXX/mangle.cpp

805

Looks like we need the single-step-desugaring loop below the big comment you quoted to stop when it hits a decltype type. That's presumably where we step from the instantiation-dependent-but-not-type-dependent decltype node to its desugared type.

clang/test/SemaTemplate/dependent-expr.cpp

132

Right. We treat statement-expressions in a template as always being value- and instantiation-dependent, in part to match GCC and in part to avoid needing to define value- and instantiation-dependence for all statements and block-scope declarations, so decltype(statement expression) would have been a dependent type before.

[rsmith]

clang/test/CodeGenCXX/mangle.cpp

805

Also... the FIXME from that comment will apply here too. Given:

template<typename T> void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
template void f<int>(unsigned long, unsigned long);

we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more. We could fix that by deduplicating DecltypeTypes when they're instantiation dependent but not dependent; that's what we do for ConstantArrayType's size expression, for example. If we do that, we'll need to store the actual expression on the DecltypeTypeLoc, since we can't rely on the DecltypeType having the right expression any more (only an expression that's equivalent to it).

[hokein]

address comments:

• don't break existing mangling tests
• deduplicate instantiation-dependent-but-not-type-dependent DecltypeType node (for name mangling substitution)
• fix the dependence bit of DecltypeType

[hokein]

clang/lib/AST/Type.cpp

3429

I think I must have missed to reset the dependent bit from the result of toTypeDependence. toTypeDependence will set the dependent bit if E is value-dependent, but it is wrong for decltype.

clang/test/CodeGenCXX/mangle.cpp

805

Thanks for the hints and explanations.

we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more.

thanks for the example, yeah, the behavior was changed with the prior change of this patch.

We could fix that by deduplicating DecltypeTypes when they're instantiation dependent but not dependent; that's what we do for ConstantArrayType's size expression, for example. If we do that, we'll need to store the actual expression on the DecltypeTypeLoc, since we can't rely on the DecltypeType having the right expression any more (only an expression that's equivalent to it).

It took me sometime to understand the whole piece here, but I'm afraid that I still don't fully understand the second part -- any reason why we can't rely on the underlying expression of DecltypeType when doing the deduplication? If we store the actual expression on DecltypeTypeLoc, how do we retrieve it in ASTContext::getDecltypeType(), I didn't find any connection between DecltypeTypeLoc and ASTContext::getDecltypeType.

I updated the patch using the DecltypeTypeLoc to do the duplication, it passes the example you gave above, but it might be wrong.

[rsmith]

clang/include/clang/AST/Type.h

4478 ↗

(On Diff #292784)

We shouldn't store the context here. To provide it to the Profile implementation, use a ContextualFoldingSet.

4499–4508 ↗

(On Diff #292784)

Can we remove this class now?

clang/lib/AST/ASTContext.cpp

5342–5346

Maybe fold these two paragraphs together

Unlike many "get<Type>" functions, we don't unique DecltypeType nodes unless they are instantiation-dependent. [...]

clang/test/CodeGenCXX/mangle.cpp

805

The problem with the patch as it now stands is that for a case such as:

int x;
template<typename T> void f(decltype(sizeof(T.f(x))));
template<typename T> void g(decltype(sizeof(T.f(x))));

... there is only one reference to x reachable in the AST, on line #2. The expression containing the DeclRefExpr on line #3 was discarded entirely. So (for example) any tooling that's looking to find references to x will not find the one on line #3.

The best way to fix this would be to store the Expr* for the actual expression in the DecltypeTypeLoc information. We'll also need to make sure that TreeTransform uses that Expr* rather than the one from the type when transforming a DecltypeType with source info.

[hokein]

store the actual expression in DecltypeTypeLoc and address comments.

[hokein]

clang/include/clang/AST/Type.h

4499–4508 ↗

(On Diff #292784)

yeah I think it is possible, but I would rather do it in a follow-up patch -- the current patch seems large enough. Added a FIXME.

clang/test/CodeGenCXX/mangle.cpp

805

Thanks, now I get it.

I have updated this patch to implement what you suggested (storing the actual expression in DecltypeTypeLoc), it touches more files. Please take another look.

[hokein]

friendly ping @rsmith :)

[hokein]

rebase

[hokein]

@rsmith I think it still makes sense to get this patch landed.

[hokein]

Closing it as Richard has landed a better patch in https://github.com/llvm/llvm-project/commit/638867afd4bce4a2c56dea041299428af3727d61.

clang/lib/AST/ASTContext.cpp

 /// on canonical types (which are always unique).
 QualType ASTContext::getTypeOfType(QualType tofType) const {
   QualType Canonical = getCanonicalType(tofType);
   auto *tot = new (*this, TypeAlignment) TypeOfType(tofType, Canonical);
   Types.push_back(tot);
   return QualType(tot, 0);
 }
 
 /// Unlike many "get<Type>" functions, we don't unique DecltypeType
 /// nodes. This would never be helpful, since each such type has its own
 /// expression, and would not give a significant memory saving, since there
 /// is an Expr tree under each such type.
 QualType ASTContext::getDecltypeType(Expr *e, QualType UnderlyingType) const {
   DecltypeType *dt;
 
-  // C++11 [temp.type]p2:
-  //   If an expression e involves a template parameter, decltype(e) denotes a
+  // C++17 [temp.type]p2:
+  //   If an expression e is type-dependent (17.6.2.2), decltype(e) denotes a
   //   unique dependent type. Two such decltype-specifiers refer to the same
-  //   type only if their expressions are equivalent (14.5.6.1).
-  if (e->isInstantiationDependent()) {
+  //   type only if their expressions are equivalent (17.5.6.1).
+  if (e->isTypeDependent()) {
     llvm::FoldingSetNodeID ID;
     DependentDecltypeType::Profile(ID, *this, e);
 
     void *InsertPos = nullptr;
     DependentDecltypeType *Canon
       = DependentDecltypeTypes.FindNodeOrInsertPos(ID, InsertPos);
     if (!Canon) {
       // Build a new, canonical decltype(expr) type.

clang/lib/AST/Type.cpp

 }
 
 void DependentTypeOfExprType::Profile(llvm::FoldingSetNodeID &ID,
                                       const ASTContext &Context, Expr *E) {
   E->Profile(ID, Context, true);
 }
 
 DecltypeType::DecltypeType(Expr *E, QualType underlyingType, QualType can)
-    // C++11 [temp.type]p2: "If an expression e involves a template parameter,
-    // decltype(e) denotes a unique dependent type." Hence a decltype type is
-    // type-dependent even if its expression is only instantiation-dependent.
+    // C++17 [temp.type]p2: "If an expression e is type-dependent, decltype(e)
+    // denotes a unique dependent type."
     : Type(Decltype, can,
            toTypeDependence(E->getDependence()) |
-               (E->isInstantiationDependent() ? TypeDependence::Dependent
-                                              : TypeDependence::None) |
+               (E->isTypeDependent() ? TypeDependence::Dependent
                (E->getType()->getDependence() &
                 TypeDependence::VariablyModified)),
       E(E), UnderlyingType(underlyingType) {}
 
-bool DecltypeType::isSugared() const { return !E->isInstantiationDependent(); }
+bool DecltypeType::isSugared() const { return !E->isTypeDependent(); }
 
 QualType DecltypeType::desugar() const {
   if (isSugared())
     return getUnderlyingType();
 
   return QualType(this, 0);
 }
 

clang/test/CodeGenCXX/mangle-exprs.cpp

namespace test4 {
   // CHECK: void @_ZN5test43tf2INS_1XEEEvDTnw_T_piilLi1EEEE
   template void tf2<X>(X*);
 
   // CHECK: void @_ZN5test43tf3INS_1XEEEvDTnw_T_ilLi1EEE
   template void tf3<X>(X*);
 
 }
 
 namespace test5 {
   template <typename T> void a(decltype(noexcept(T()))) {}
   template void a<int>(decltype(noexcept(int())));
-  // CHECK: void @_ZN5test51aIiEEvDTnxcvT__EE(
+  // CHECK: void @_ZN5test51aIiEEvb
 }
 
 namespace test6 {
   struct X {
     int i;
   };
 
   struct Y {

clang/test/CodeGenCXX/mangle.cpp

namespace test33 {
   }
 }
 
 namespace test34 {
   // Mangling for instantiation-dependent decltype expressions.
   template<typename T>
   void f(decltype(sizeof(decltype(T() + T())))) {}
 
-  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f<int>(decltype(sizeof(1)));
 
   // Mangling for non-instantiation-dependent sizeof expressions.
   template<unsigned N>
   void f2(int (&)[N + sizeof(int*)]) {}
 
   // CHECK-LABEL: define weak_odr void @_ZN6test342f2ILj4EEEvRAplT_Lm8E_i
   template void f2<4>(int (&)[4 + sizeof(int*)]);
namespace test35 {
   // Dependent operator names of unknown arity.
   struct A { 
     template<typename U> A operator+(U) const;
   };
 
   template<typename T>
   void f1(decltype(sizeof(&T::template operator+<int>))) {}
 
-  // CHECK-LABEL: define weak_odr void @_ZN6test352f1INS_1AEEEvDTszadsrT_onplIiEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test352f1INS_1AEEEvm
   template void f1<A>(__SIZE_TYPE__);
 }
 
 namespace test36 {
   template<unsigned> struct A { };
 
   template<typename ...Types>
   auto f1(Types... values) -> A<sizeof...(values)> { }

 
 template void fn<E>(E, __underlying_type(E));
 // CHECK-LABEL: @_ZN6test552fnINS_1EEEEvT_U3eutS2_
 }
 
 namespace test56 {
   struct A { A *operator->(); int n; } a;
   template<int N> void f(decltype(a->n + N)) {}
-  // CHECK-LABEL: @_ZN6test561fILi0EEEvDTplptL_ZNS_1aEE1nT_E
+  // CHECK-LABEL: @_ZN6test561fILi0EEEvi
   template void f<0>(int);
 }
 
 namespace test57 {
   struct X { template <int N> int f(); } x;
   template<int N> void f(decltype(x.f<0>() + N)) {}
-  // CHECK-LABEL: @_ZN6test571fILi0EEEvDTplcldtL_ZNS_1xEE1fIXLi0EEEET_E
+  // CHECK-LABEL: @_ZN6test571fILi0EEEvi
   template void f<0>(int);
 }
 
 namespace test58 {
   struct State {
    bool m_fn1();
   } a;
   template <class T> struct identity { typedef T type; };

clang/test/SemaCXX/invalid-template-base-specifier.cpp

 // RUN: %clang_cc1 -frecovery-ast -verify %s
 
 bool Foo(int *); // expected-note 3{{candidate function not viable}}
 
 template <typename T>
 struct Crash : decltype(Foo(T())) { // expected-error {{no matching function for call to 'Foo'}}
   Crash(){};
 };
 
 void test() { Crash<int>(); } // expected-note {{in instantiation of template class}}
 
 template <typename T>
 using Alias = decltype(Foo(T())); // expected-error {{no matching function for call to 'Foo'}}
 template <typename T>
-struct Crash2 : decltype(Alias<T>()) { // expected-note {{in instantiation of template type alias 'Alias' requested here}}
+struct Crash2 : decltype(Alias<T>()) { // expected-note {{in instantiation of template type alias 'Alias' requested here}} \
+                                          expected-error {{base specifier must name a class}}
   Crash2(){};
 };
 
-void test2() { Crash2<int>(); } // expected-note {{in instantiation of template class 'Crash2<int>' requested here}}
+void test2() { Crash2<int>(); } // expected-note2 {{in instantiation of template class 'Crash2<int>' requested here}}
 
 template <typename T>
 class Base {};
 template <typename T>
 struct Crash3 : Base<decltype(Foo(T()))> { // expected-error {{no matching function for call to 'Foo'}}
   Crash3(){};
 };
 
 void test3() { Crash3<int>(); } // expected-note {{in instantiation of template class}}

clang/test/SemaTemplate/dependent-expr.cpp

template<typename> struct B : A {
     }
   };
 
   template void B<int>::f();
 
   template<typename> void f() {
     decltype(({})) x; // expected-error {{incomplete type}}
   }
-  template void f<int>(); // expected-note {{instantiation of}}
+  template void f<int>();
 
   template<typename> auto g() {
     auto c = [](auto, int) -> decltype(({})) {};
     using T = decltype(c(0.0, 0));
     using T = void;
     return c(0, 0);
   }
   using U = decltype(g<int>()); // expected-note {{previous}}