[clang] Fix serializers for `TypeTemplateParmDecl` + related types

[elsteveogrande]

Summary

Addresses issue: https://bugs.llvm.org/show_bug.cgi?id=34728

Template type parameters can optionally have default values; at the first parameter declaration at which the default is declared, e.g. i in template <int i=0, class T>, this decl is treated as the "owner" of the default value, in this case 0.

However the serializer originally didn't record inherited-from-decl (owner) info at non-owners, only the default value in the owner's serialized bytes. So subsequent template declarations that should inherit this default value wouldn't, when used in a PCH or some serialized form.

For example, a precompiled header containing this:

template <int foo=1, class T> int func(T const&);
template <int foo, class T> int func(T const&) { return foo; }

would not work in this source file, if used with PCH (but it would if it were included as an ordinary, non-pre-compiled header):

int main(int, char**) { return func(1.23); }

because of a spurious error about not being able to deduce foo, having lost the default value inheritance info during serialization:

candidate template ignored: couldn't infer template argument 'foo'

Tested with:

• New test cases added, now passing.
• Existing test cases look ok. (ninja check-clang)

[elsteveogrande]

I submitted this patch relative to the clang project root, hopefully that's ok.

[elsteveogrande]

Hi @bruno, @rsmith, does this approach look ok?

I couldn't easily figure out a better way to store inherited-default-info; doing it alongside the default value seemed the cleanest.

Note: I saw there are three ways to store these data inside the DefaultArgStorage struct, see here:
https://reviews.llvm.org/diffusion/L/browse/cfe/trunk/include/clang/AST/DeclTemplate.h;315074$272

There's (1) the default arg; (2) the reference to the Decl having the default arg (inherited by this Decl); and (3) a pointer to a Chain link, which is the case when modules are used.

The serializer already had (1) covered, and I'm adding (2) here. Maybe the most proper fix is to handle (3) as well. Although it seems no unit tests broke, so this is either ok as is, or there's no test for it :)

So: right now the serialization stream generated in ASTWriterDecl.cpp looks sort of like:

 [ . . . other fields]
bool defaultArgFollows
[Stmt defaultArgStmtRef (if above is true)]
bool inheritedFromDeclFollows
[Decl inheritedFromDecl (if above is true)]

This could be changed to:

 [ . . . other fields]
bool isPartOfChain     <----- add this
bool defaultArgFollows
[Stmt defaultArgStmtRef (if above is true)]
bool inheritedFromDeclFollows
[Decl inheritedFromDecl (if above is true)]

so this can store the DefaultArgStorage with better fidelity. Thoughts?

Thanks! :)

[arphaman]

Hi,

Thanks for your patch! Could you please post a patch with full context (git diff -U99999)?

lib/Serialization/ASTWriterDecl.cpp

1541

Please start your variable names in uppercase (see http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

1551

I think that cast to bool here and below is redundant.

[elsteveogrande]

Hi @arphaman, thanks very much! Sorry, I didn't notice there was activity on this old patch until now.

I'll make these changes as suggested and re-send it with full context.

[elsteveogrande]

Fixed local var names. Using arc diff which hopefully submits latest changes w/ full context

[elsteveogrande]

Update: now works properly with modules as well.

This needed to handle the case where a ParmDecl is already inheriting, but it's innocuously inheriting from the same decl. Now inheritDefaultTemplateArgument will return successfully and avoid the assertion error saying that default argument already inherited.

We'll now possibly step once or twice through the inheritance list, so when a ParmDecl is inheriting from another ParmDecl, which *itself* is inheriting from a third decl. This is the case where inheritDefaultTemplateArgument is called with From being an inheriting (and not "owning") template parameter.

Modules make use of the Chain struct as well, and IIUC this can lead to one extra level of indirection, as it too has a ParmDecl * field. So this now handles up to 2 indirections before validating that the inheritance is consistent.

Passes existing and new tests; also works with a mini project that uses modules, which was previously broken.

[elsteveogrande]

Rebase very old diff

[elsteveogrande]

cc @rsmith as this has to do with modules.

Verified that (1) existing tests pass on trunk; (2) the new test included here fails when applied to trunk; (3) all tests (including new test) pass when this patch is applied.

[elsteveogrande]

hi @rsmith have a look and let me know if this change looks sensible :) Please make sure I have the right mental model for inheritance in the module case

[rsmith]

lib/Serialization/ASTReaderDecl.cpp

3448–3456

The idea is to reconstruct the 'inherited default template argument' information when we read a template declaration and find it's a redeclaration of another one, rather than serializing it and deserializing it. (In particular, if an imported template redeclares a non-imported template or a template imported from an unrelated module, it won't know that the prior declaration had a default argument because it doesn't know what the prior declaration was.)

The problem with our implementation of that idea is these breaks: we're (incorrectly) assuming that a template parameter cannot have a default argument if there's a later template parameter that does not have one.

I removed the breaks in r338438, and it fixed your testcase (which I committed alongside that change). Can you check to see if that also fixes the original problem from which this was reduced? Thanks!

[elsteveogrande]

lib/Serialization/ASTReaderDecl.cpp

3448–3456

Thanks @rsmith! It sounds like that ought to do it. We'll try again on our end and let you know. Thanks for the quick fix!

[elsteveogrande]

Dropped in favor of other fix as mentioned above

lib/Serialization/ASTReaderDecl.cpp

 };
 } // end anonymous namespace
 
+/// Inherit the default template argument from \p From to \p To. Returns
+/// \c false if there is no default template for \p From.
+/// It is an error to call this method more than once on \p To, with differing
+/// \p From decls.
+template <typename ParmDecl>
+static bool inheritDefaultTemplateArgument(ASTContext &Context, ParmDecl *From,
+                                           Decl *ToD);
+
 template <typename DeclT>
 static llvm::iterator_range<MergedRedeclIterator<DeclT>>
 merged_redecls(DeclT *D) {

 
   if (Record.readInt())
     D->setDefaultArgument(GetTypeSourceInfo());
+
+  if (Record.readInt()) {
+    inheritDefaultTemplateArgument(
+        Reader.getContext(), Record.readDeclAs<TemplateTypeParmDecl>(), D);
+  }
 }
 
 void ASTDeclReader::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {

   } else {
     // Rest of NonTypeTemplateParmDecl.
     D->ParameterPack = Record.readInt();
+
     if (Record.readInt())
       D->setDefaultArgument(Record.readExpr());
+
+    if (Record.readInt()) {
+      inheritDefaultTemplateArgument(
+          Reader.getContext(), Record.readDeclAs<NonTypeTemplateParmDecl>(), D);
+    }
   }
 }
 

   } else {
     // Rest of TemplateTemplateParmDecl.
     D->ParameterPack = Record.readInt();
+
     if (Record.readInt())
       D->setDefaultArgument(Reader.getContext(),
                             Record.readTemplateArgumentLoc());
+
+    if (Record.readInt()) {
+      inheritDefaultTemplateArgument(
+          Reader.getContext(), Record.readDeclAs<TemplateTemplateParmDecl>(),
+          D);
+    }
   }
 }
 

 
 /// Inherit the default template argument from \p From to \p To. Returns
 /// \c false if there is no default template for \p From.
+/// It is an error to call this method more than once on \p To, with differing
+/// \p From decls.
 template <typename ParmDecl>
 static bool inheritDefaultTemplateArgument(ASTContext &Context, ParmDecl *From,
                                            Decl *ToD) {
   auto *To = cast<ParmDecl>(ToD);
   if (!From->hasDefaultArgument())
     return false;
+  auto const &DA = To->getDefaultArgStorage();
+  if (DA.isSet() && DA.isInherited()) {
+    assert(DA.getInheritedFrom() == From &&
+           "template arg already inherits from a different decl");
+    return true;
+  }
   To->setInheritedDefaultArgument(Context, From);
   return true;
 }

lib/Serialization/ASTWriterDecl.cpp

 
   Record.push_back(D->wasDeclaredWithTypename());
 
-  bool OwnsDefaultArg = D->hasDefaultArgument() &&
-                        !D->defaultArgumentWasInherited();
-  Record.push_back(OwnsDefaultArg);
-  if (OwnsDefaultArg)
-    Record.AddTypeSourceInfo(D->getDefaultArgumentInfo());
+  TypeSourceInfo *ownedDefaultArg = nullptr;
+  Decl const *inheritsFromDecl = nullptr;
+  if (D->hasDefaultArgument()) {
+    if (D->defaultArgumentWasInherited()) {
+      inheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+    } else {
+      ownedDefaultArg = D->getDefaultArgumentInfo();
+    }
+  }
+
+  Record.push_back(bool(ownedDefaultArg != nullptr));
+  if (ownedDefaultArg) {
+    Record.AddTypeSourceInfo(ownedDefaultArg);
+  }
+  Record.push_back(bool(inheritsFromDecl != nullptr));
+  if (inheritsFromDecl) {
+    Record.AddDeclRef(inheritsFromDecl);
+  }
 
   Code = serialization::DECL_TEMPLATE_TYPE_PARM;
 }

   } else {
     // Rest of NonTypeTemplateParmDecl.
     Record.push_back(D->isParameterPack());
-    bool OwnsDefaultArg = D->hasDefaultArgument() &&
-                          !D->defaultArgumentWasInherited();
-    Record.push_back(OwnsDefaultArg);
-    if (OwnsDefaultArg)
-      Record.AddStmt(D->getDefaultArgument());
+
+    Expr *ownedDefaultArg = nullptr;
+    Decl const *inheritsFromDecl = nullptr;
+    if (D->hasDefaultArgument()) {
+      if (D->defaultArgumentWasInherited()) {
+        inheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+      } else {
+        ownedDefaultArg = D->getDefaultArgument();
+      }
+    }
+
+    Record.push_back(bool(ownedDefaultArg != nullptr));
+    if (ownedDefaultArg) {
+      Record.AddStmt(ownedDefaultArg);
+    }
+    Record.push_back(bool(inheritsFromDecl != nullptr));
+    if (inheritsFromDecl) {
+      Record.AddDeclRef(inheritsFromDecl);
+    }
+
     Code = serialization::DECL_NON_TYPE_TEMPLATE_PARM;
   }
 }

   } else {
     // Rest of TemplateTemplateParmDecl.
     Record.push_back(D->isParameterPack());
-    bool OwnsDefaultArg = D->hasDefaultArgument() &&
-                          !D->defaultArgumentWasInherited();
-    Record.push_back(OwnsDefaultArg);
-    if (OwnsDefaultArg)
-      Record.AddTemplateArgumentLoc(D->getDefaultArgument());
+
+    llvm::Optional<TemplateArgumentLoc> ownedDefaultArg;
+    Decl const *inheritsFromDecl = nullptr;
+    if (D->hasDefaultArgument()) {
+      if (D->defaultArgumentWasInherited()) {
+        inheritsFromDecl = D->getDefaultArgStorage().getInheritedFrom();
+      } else {
+        ownedDefaultArg = D->getDefaultArgument();
+      }
+    }
+
+    Record.push_back(bool(ownedDefaultArg.hasValue()));
+    if (ownedDefaultArg.hasValue()) {
+      Record.AddTemplateArgumentLoc(*ownedDefaultArg);
+    }
+    Record.push_back(bool(inheritsFromDecl != nullptr));
+    if (inheritsFromDecl) {
+      Record.AddDeclRef(inheritsFromDecl);
+    }
+
     Code = serialization::DECL_TEMPLATE_TEMPLATE_PARM;
   }
 }

test/PCH/cxx-templates.h

 namespace MemberSpecializationLocation {
   template<typename T> struct A { static int n; };
 }
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+
+// case 1: defaulted `NonTypeTemplateParmDecl`, non-defaulted 2nd tpl param
+template <int foo = 42, class T>
+int func1(T const &);
+
+template <int foo, class T>
+int func1(T const &) {
+  return foo;
+}
+
+// case 2: defaulted `TemplateTypeParmDecl`, non-defaulted 2nd tpl param
+template <class A = int, class B>
+A func2(B const &);
+
+template <class A, class B>
+A func2(B const &) {
+  return 69;
+}
+
+// case 3: defaulted `TemplateTemplateParmDecl`, non-defaulted 2nd tpl param
+template <class T>
+struct Container { T const &item; };
+
+template <template <class> class C = Container, class D>
+C<D> func3(D const &);
+
+template <template <class> class C, class D>
+C<D> func3(D const &d) {
+  return Container<D>{d};
+}
+
+} // end namespace PR34728

test/PCH/cxx-templates.cpp

 #endif
   int k = A<int>::n;
 }
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+int test() {
+  int z1 = func1(2.718);
+  int z2 = func2(3.142);
+  int z3 = 12;
+  Container<int> c = func3(z3);
+  return z1 + z2 + c.item;
+}
+} // end namespace PR34728