[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

3404–3408

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 ↗

(On Diff #290653)

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 ↗

(On Diff #290653)

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

3404–3408

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

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

4499–4508

Can we remove this class now?

clang/lib/AST/ASTContext.cpp

5342–5347

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

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/include/clang/AST/ASTContext.h

class ASTContext : public RefCountedBase<ASTContext> {
   mutable llvm::FoldingSet<DependentVectorType> DependentVectorTypes;
   mutable llvm::FoldingSet<ConstantMatrixType> MatrixTypes;
   mutable llvm::FoldingSet<DependentSizedMatrixType> DependentSizedMatrixTypes;
   mutable llvm::FoldingSet<FunctionNoProtoType> FunctionNoProtoTypes;
   mutable llvm::ContextualFoldingSet<FunctionProtoType, ASTContext&>
     FunctionProtoTypes;
   mutable llvm::FoldingSet<DependentTypeOfExprType> DependentTypeOfExprTypes;
   mutable llvm::FoldingSet<DependentDecltypeType> DependentDecltypeTypes;
+  mutable llvm::FoldingSet<DecltypeType> DecltypeTypes;
   mutable llvm::FoldingSet<TemplateTypeParmType> TemplateTypeParmTypes;
   mutable llvm::FoldingSet<ObjCTypeParamType> ObjCTypeParamTypes;
   mutable llvm::FoldingSet<SubstTemplateTypeParmType>
     SubstTemplateTypeParmTypes;
   mutable llvm::FoldingSet<SubstTemplateTypeParmPackType>
     SubstTemplateTypeParmPackTypes;
   mutable llvm::ContextualFoldingSet<TemplateSpecializationType, ASTContext&>
     TemplateSpecializationTypes;

clang/include/clang/AST/Type.h

public:
 
   /// Returns whether this type directly provides sugar.
   bool isSugared() const { return true; }
 
   static bool classof(const Type *T) { return T->getTypeClass() == TypeOf; }
 };
 
 /// Represents the type `decltype(expr)` (C++11).
-class DecltypeType : public Type {
+class DecltypeType : public Type, public llvm::FoldingSetNode {
   Expr *E;
   QualType UnderlyingType;
 
 protected:
   friend class ASTContext; // ASTContext creates these.
 
-  DecltypeType(Expr *E, QualType underlyingType, QualType can = QualType());
+  const ASTContext &Context;
+  DecltypeType(Expr *E, QualType underlyingType, const ASTContext &Context,
+               QualType can = QualType());
 
 public:
   Expr *getUnderlyingExpr() const { return E; }
   QualType getUnderlyingType() const { return UnderlyingType; }
 
   /// Remove a single level of sugar.
   QualType desugar() const;
 
   /// Returns whether this type directly provides sugar.
   bool isSugared() const;
 
   static bool classof(const Type *T) { return T->getTypeClass() == Decltype; }
+
+  void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, Context, E); }
+  static void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
+                      Expr *E);
 };
 
 /// Internal representation of canonical, dependent
 /// decltype(expr) types.
 ///
 /// This class is used internally by the ASTContext to manage
 /// canonical, dependent types, only. Clients will only see instances
 /// of this class via DecltypeType nodes.
-class DependentDecltypeType : public DecltypeType, public llvm::FoldingSetNode {
-  const ASTContext &Context;
-
+class DependentDecltypeType : public DecltypeType {
 public:
   DependentDecltypeType(const ASTContext &Context, Expr *E);
-
-  void Profile(llvm::FoldingSetNodeID &ID) {
-    Profile(ID, Context, getUnderlyingExpr());
-  }
-
-  static void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-                      Expr *E);
 };
 
 /// A unary type transform, which is a type constructed from another.
 class UnaryTransformType : public Type {
 public:
   enum UTTKind {
     EnumUnderlyingType
   };
 

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.
+/// The instantiation-dependent-but-not-type-dependent DecltypeType node is an
+/// exception, we unique it for forming correct substitutions in name mangling.
 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.
       Canon = new (*this, TypeAlignment) DependentDecltypeType(*this, e);
       DependentDecltypeTypes.InsertNode(Canon, InsertPos);
     }
-    dt = new (*this, TypeAlignment)
-        DecltypeType(e, UnderlyingType, QualType((DecltypeType *)Canon, 0));
+    dt = new (*this, TypeAlignment) DecltypeType(
+        e, UnderlyingType, *this, QualType((DecltypeType *)Canon, 0));
+  } else if (e->isInstantiationDependent()) {
+    llvm::FoldingSetNodeID ID;
+    DecltypeType::Profile(ID, *this, e);
+    void *InsertPos = nullptr;
+    if (DecltypeType *DT = DecltypeTypes.FindNodeOrInsertPos(ID, InsertPos))
+      return QualType(DT, 0);
+    dt = new (*this, TypeAlignment) DecltypeType(
+        e, UnderlyingType, *this, getCanonicalType(UnderlyingType));
+    DecltypeTypes.InsertNode(dt, InsertPos);
   } else {
-    dt = new (*this, TypeAlignment)
-        DecltypeType(e, UnderlyingType, getCanonicalType(UnderlyingType));
+    dt = new (*this, TypeAlignment) DecltypeType(
+        e, UnderlyingType, *this, getCanonicalType(UnderlyingType));
   }
   Types.push_back(dt);
   return QualType(dt, 0);
 }
 
 /// getUnaryTransformationType - We don't unique these, since the memory
 /// savings are minimal and these are rare.
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

clang/lib/AST/ItaniumMangle.cpp

 static bool isTypeSubstitutable(Qualifiers Quals, const Type *Ty,
                                 ASTContext &Ctx) {
   if (Quals)
     return true;
   if (Ty->isSpecificBuiltinType(BuiltinType::ObjCSel))
     return true;
   if (Ty->isOpenCLSpecificType())
     return true;
+  if (Ty->isDecltypeType() && Ty->isInstantiationDependentType())
+    return true;
   if (Ty->isBuiltinType())
     return false;
   // Through to Clang 6.0, we accidentally treated undeduced auto types as
   // substitution candidates.
   if (Ctx.getLangOpts().getClangABICompat() > LangOptions::ClangABI::Ver6 &&
       isa<AutoType>(Ty))
     return false;
   return true;
else {
     do {
       // Don't desugar through template specialization types that aren't
       // type aliases. We need to mangle the template arguments as written.
       if (const TemplateSpecializationType *TST
                                       = dyn_cast<TemplateSpecializationType>(T))
         if (!TST->isTypeAlias())
           break;
 
+      // Don't desugar through instantiation-dependent-but-not-type-dependent
+      // DecltypeType, as we want to mangle it as written.
+      if (const auto *DT = dyn_cast<DecltypeType>(T))
+        if (!DT->isDependentType() && DT->isInstantiationDependentType())
+          break;
+
       QualType Desugared
         = T.getSingleStepDesugaredType(Context.getASTContext());
       if (Desugared == T)
         break;
 
       T = Desugared;
     } while (true);
   }

clang/lib/AST/Type.cpp

QualType TypeOfExprType::desugar() const {
   return QualType(this, 0);
 }
 
 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.
+DecltypeType::DecltypeType(Expr *E, QualType underlyingType,
+                           const ASTContext &Context, QualType can)
+    // 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) |
+           // C++17 [temp.dep.type]p9: "denoted by decltype(expression), where
+           // expression is type-dependent"
+           (toTypeDependence(E->getDependence()) & ~TypeDependence::Dependent) |
+               (E->isTypeDependent() ? TypeDependence::Dependent
                (E->getType()->getDependence() &
                 TypeDependence::VariablyModified)),
-      E(E), UnderlyingType(underlyingType) {}
+      E(E), UnderlyingType(underlyingType), Context(Context) {}
 
-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);
 }
 
-DependentDecltypeType::DependentDecltypeType(const ASTContext &Context, Expr *E)
-    : DecltypeType(E, Context.DependentTy), Context(Context) {}
-
-void DependentDecltypeType::Profile(llvm::FoldingSetNodeID &ID,
-                                    const ASTContext &Context, Expr *E) {
+void DecltypeType::Profile(llvm::FoldingSetNodeID &ID,
   E->Profile(ID, Context, true);
 }
 
+DependentDecltypeType::DependentDecltypeType(const ASTContext &Context, Expr *E)
+    : DecltypeType(E, Context.DependentTy, Context) {}
+
 UnaryTransformType::UnaryTransformType(QualType BaseType,
                                        QualType UnderlyingType, UTTKind UKind,
                                        QualType CanonicalType)
     : Type(UnaryTransform, CanonicalType, BaseType->getDependence()),
       BaseType(BaseType), UnderlyingType(UnderlyingType), UKind(UKind) {}
 
 DependentUnaryTransformType::DependentUnaryTransformType(const ASTContext &C,
                                                          QualType BaseType,

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
   template void f<int>(decltype(sizeof(1)));
 
+  template <typename T> void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvDTstT_ES2_
+  template void f<int>(unsigned long, unsigned long);
+
   // 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*)]);
 
   // Mangling for non-instantiation-dependent sizeof expressions

clang/test/Sema/invalid-bitwidth-expr.mm

 constexpr int sss = sizeof(Y);
 
 bool Foo(int *); // expected-note {{candidate function not viable}}
 template <typename T>
 struct Base {};
 template <typename T>
 auto func() {
   // error-bit should be propagated from TemplateArgument to NestNameSpecifier.
-  class Base<decltype(Foo(T()))>::type C; // expected-error {{no matching function for call to 'Foo'}}
+  class Base<decltype(Foo(T()))>::type C; // expected-error {{no matching function for call to 'Foo'}} \
+                                             expected-error {{no class named 'type' in}}
   return C;
 }
 struct Z {
   int X : func<int>(); // expected-note {{in instantiation of function template}}
 };
 constexpr int ssss = sizeof(Z);
 
 struct Z2 {
   int X : sizeof(_ExtInt(invalid())); // expected-error {{use of undeclared identifier}}
 };
 constexpr int sssss = sizeof(Z2);

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}}

clang/test/SemaTemplate/temp_arg_template_cxx1z.cpp

namespace Auto {
   // Perhaps this case should be invalid, as there are valid 'decltype(auto)'
   // parameters (such as 'user-defined-type &') that are not valid 'auto'
   // parameters.
   TDecltypeAuto<Auto> daa;
   TDecltypeAuto<AutoPtr> daap; // FIXME: should probably be ill-formed
 
   int n;
   template<auto A, decltype(A) B = &n> struct SubstFailure;
-  TInt<SubstFailure> isf; // FIXME: this should be ill-formed
+  TInt<SubstFailure> isf; // expected-error {{template template argument has different template parameters than its corresponding template template parameter}}
   TIntPtr<SubstFailure> ipsf;
 }