[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

public:
   friend bool operator!=(const MergedRedeclIterator &A,
                          const MergedRedeclIterator &B) {
     return A.Current != B.Current;
   }
 };
 
 } // namespace
 
+/// Inherit the default template argument from \p From to \p To. Returns
+/// \c false if there is no default template for \p From.
+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) {
   return llvm::make_range(MergedRedeclIterator<DeclT>(D),
                           MergedRedeclIterator<DeclT>());
 }
 
 uint64_t ASTDeclReader::GetCurrentCursorOffset() {

 
 void ASTDeclReader::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   VisitTypeDecl(D);
 
   D->setDeclaredWithTypename(Record.readInt());
 
   if (Record.readInt())
     D->setDefaultArgument(GetTypeSourceInfo());
+
+  if (Record.readInt()) {
+    inheritDefaultTemplateArgument(
+        Reader.getContext(), Record.readDeclAs<TemplateTypeParmDecl>(), D);
+  }
 }
 
 void ASTDeclReader::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
   VisitDeclaratorDecl(D);
   // TemplateParmPosition.
   D->setDepth(Record.readInt());
   D->setPosition(Record.readInt());
   if (D->isExpandedParameterPack()) {
     auto TypesAndInfos =
         D->getTrailingObjects<std::pair<QualType, TypeSourceInfo *>>();
     for (unsigned I = 0, N = D->getNumExpansionTypes(); I != N; ++I) {
       new (&TypesAndInfos[I].first) QualType(Record.readType());
       TypesAndInfos[I].second = GetTypeSourceInfo();
     }
   } 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);
+    }
   }
 }
 
 void ASTDeclReader::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
   VisitTemplateDecl(D);
   // TemplateParmPosition.
   D->setDepth(Record.readInt());
   D->setPosition(Record.readInt());
   if (D->isExpandedParameterPack()) {
     auto **Data = D->getTrailingObjects<TemplateParameterList *>();
     for (unsigned I = 0, N = D->getNumExpansionTemplateParameters();
          I != N; ++I)
       Data[I] = Record.readTemplateParameterList();
   } 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);
+    }
   }
 }
 
 void ASTDeclReader::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
   VisitRedeclarableTemplateDecl(D);
 }
 
 void ASTDeclReader::VisitStaticAssertDecl(StaticAssertDecl *D) {

 }
 
 } // namespace clang
 
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader, ...) {
   llvm_unreachable("attachPreviousDecl on non-redeclarable declaration");
 }
 
+/// Given some kind of a template parameter having a default argument, find the
+/// decl which actually provides the default argument, if inherited; otherwise,
+/// simply return \b D.
+template <typename ParmDecl>
+static ParmDecl const *getDefaultArgOwner(ParmDecl const *D,
+                                          unsigned MaxRecurse = 2) {
+  assert(D != nullptr && "getDefaultArgOwner on a null decl");
+  auto const &DA = D->getDefaultArgStorage();
+  assert(DA.isSet() && "getDefaultArgOwner on a decl with no default arg");
+  auto const *Prev = DA.getInheritedFrom();
+  if (Prev == nullptr || Prev == D) {
+    return D;
+  }
+  // Shouldn't have to pointer-chase too much, so we'll institute a limit of
+  // two dereferences:
+  // * one for the case where there's an inherited parameter, i.e. `DA`'s value
+  //   for `ValueOrInherited` is a `ParmDecl *`, which should always be the
+  //   owner of the default parameter;
+  // * one for the modules case, where `ValueOrInherited` is a `Chain *`, which
+  //   itself contains a pointer to a `ParmDecl *`.
+  assert(MaxRecurse && "too many levels of indirection, or inheritance cycle?");
+  return getDefaultArgOwner(Prev, MaxRecurse - 1);
+}
+
 /// Inherit the default template argument from \p From to \p To. Returns
 /// \c false if there is no default template for \p From.
 template <typename ParmDecl>
 static bool inheritDefaultTemplateArgument(ASTContext &Context, ParmDecl *From,
                                            Decl *ToD) {
-  auto *To = cast<ParmDecl>(ToD);
-  if (!From->hasDefaultArgument())
+  if (!From->hasDefaultArgument()) {
+    // Nothing to inherit; done.
     return false;
+  }
+
+  // Track down the owning decl, that is, the one which actually has the default
+  // arg, rather than one that might inherits it.  We'll inherit from the
+  // owner directly, and avoid what would be an indefinitely long chain of
+  // pointers.
+  From = const_cast<ParmDecl*>(getDefaultArgOwner(From));
+
+  auto *To = cast<ParmDecl>(ToD);
+  if (To->hasDefaultArgument() &&
+      getDefaultArgOwner(To) == From) {
+    // `To` already inherits from the same decl; done.
+    return true;
+  }
+
+  // Set up the inheritance info in `To`.  Note that this will result in a
+  // failed assertion if `To` already inherits from elsewhere, or `From` doesn't
+  // directly own that default arg.
   To->setInheritedDefaultArgument(Context, From);
+
   return true;
 }
 
 static void inheritDefaultTemplateArguments(ASTContext &Context,
                                             TemplateDecl *From,
                                             TemplateDecl *To) {
   auto *FromTP = From->getTemplateParameters();
   auto *ToTP = To->getTemplateParameters();
   assert(FromTP->size() == ToTP->size() && "merged mismatched templates?");
 
   for (unsigned I = 0, N = FromTP->size(); I != N; ++I) {
     NamedDecl *FromParam = FromTP->getParam(N - I - 1);
     if (FromParam->isParameterPack())
       continue;
     NamedDecl *ToParam = ToTP->getParam(N - I - 1);
 
     if (auto *FTTP = dyn_cast<TemplateTypeParmDecl>(FromParam)) {
       if (!inheritDefaultTemplateArgument(Context, FTTP, ToParam))
         break;
     } else if (auto *FNTTP = dyn_cast<NonTypeTemplateParmDecl>(FromParam)) {
       if (!inheritDefaultTemplateArgument(Context, FNTTP, ToParam))
         break;
     } else {
       if (!inheritDefaultTemplateArgument(
               Context, cast<TemplateTemplateParmDecl>(FromParam), ToParam))
         break;
     }
   }
 }
 
 void ASTDeclReader::attachPreviousDecl(ASTReader &Reader, Decl *D,
                                        Decl *Previous, Decl *Canon) {
   assert(D && Previous);
 

lib/Serialization/ASTWriterDecl.cpp

if (D->getPreviousDecl() == nullptr) {
     Record.AddDeclRef(D->getInstantiatedFromMember());
     Record.push_back(D->isMemberSpecialization());
   }
 
   Code = serialization::DECL_VAR_TEMPLATE_PARTIAL_SPECIALIZATION;
 }
 
 void ASTDeclWriter::VisitClassScopeFunctionSpecializationDecl(
                                     ClassScopeFunctionSpecializationDecl *D) {
   VisitDecl(D);
   Record.AddDeclRef(D->getSpecialization());
   Code = serialization::DECL_CLASS_SCOPE_FUNCTION_SPECIALIZATION;
 }
 
 
 void ASTDeclWriter::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
   VisitRedeclarableTemplateDecl(D);
 
   if (D->isFirstDecl())
     AddTemplateSpecializations(D);
   Code = serialization::DECL_FUNCTION_TEMPLATE;
 }
 
 void ASTDeclWriter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   VisitTypeDecl(D);
 
   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(OwnedDefaultArg != nullptr);
+  if (OwnedDefaultArg) {
+    Record.AddTypeSourceInfo(OwnedDefaultArg);
+  }
+  Record.push_back(InheritsFromDecl != nullptr);
+  if (InheritsFromDecl) {
+    Record.AddDeclRef(InheritsFromDecl);
+  }
 
   Code = serialization::DECL_TEMPLATE_TYPE_PARM;
 }
 
 void ASTDeclWriter::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
   // For an expanded parameter pack, record the number of expansion types here
   // so that it's easier for deserialization to allocate the right amount of
   // memory.
for (unsigned I = 0, N = D->getNumExpansionTypes(); I != N; ++I) {
       Record.AddTypeRef(D->getExpansionType(I));
       Record.AddTypeSourceInfo(D->getExpansionTypeSourceInfo(I));
     }
       
     Code = serialization::DECL_EXPANDED_NON_TYPE_TEMPLATE_PARM_PACK;
   } 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(OwnedDefaultArg != nullptr);
+    if (OwnedDefaultArg) {
+      Record.AddStmt(OwnedDefaultArg);
+    }
+    Record.push_back(InheritsFromDecl != nullptr);
+    if (InheritsFromDecl) {
+      Record.AddDeclRef(InheritsFromDecl);
+    }
+
     Code = serialization::DECL_NON_TYPE_TEMPLATE_PARM;
   }
 }
 
 void ASTDeclWriter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
   // For an expanded parameter pack, record the number of expansion types here
   // so that it's easier for deserialization to allocate the right amount of
   // memory.
   if (D->isExpandedParameterPack())
     Record.push_back(D->getNumExpansionTemplateParameters());
 
   VisitTemplateDecl(D);
   // TemplateParmPosition.
   Record.push_back(D->getDepth());
   Record.push_back(D->getPosition());
 
   if (D->isExpandedParameterPack()) {
     for (unsigned I = 0, N = D->getNumExpansionTemplateParameters();
          I != N; ++I)
       Record.AddTemplateParameterList(D->getExpansionTemplateParameters(I));
     Code = serialization::DECL_EXPANDED_TEMPLATE_TEMPLATE_PARM_PACK;
   } 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(OwnedDefaultArg.hasValue());
+    if (OwnedDefaultArg.hasValue()) {
+      Record.AddTemplateArgumentLoc(*OwnedDefaultArg);
+    }
+    Record.push_back(InheritsFromDecl != nullptr);
+    if (InheritsFromDecl) {
+      Record.AddDeclRef(InheritsFromDecl);
+    }
+
     Code = serialization::DECL_TEMPLATE_TEMPLATE_PARM;
   }
 }
 
 void ASTDeclWriter::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
   VisitRedeclarableTemplateDecl(D);
   Code = serialization::DECL_TYPE_ALIAS_TEMPLATE;
 }

test/PCH/cxx-templates.h

void crash() {
     B some;
     bar(some);
   }
 }
 
 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 = 10, 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 A(20.0f);
+}
+
+// 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

 
 namespace MemberSpecializationLocation {
 #ifndef NO_ERRORS
   // expected-note@cxx-templates.h:* {{previous}}
   template<> float A<int>::n; // expected-error {{redeclaration of 'n' with a different type}}
 #endif
   int k = A<int>::n;
 }
+
+// https://bugs.llvm.org/show_bug.cgi?id=34728
+namespace PR34728 {
+int test() {
+  // Verify with several TemplateParmDecl kinds, using PCH (incl. modules).
+  int z1 = func1(/*ignored*/2.718);
+  int z2 = func2(/*ignored*/3.142);
+  int tmp3 = 30;
+  Container<int> c = func3(tmp3);
+  int z3 = c.item;
+
+  // Return value is meaningless.  Just "use" all these values to avoid
+  // warning about unused vars / values.
+  return z1 + z2 + z3;
+}
+} // end namespace PR34728