PR28752: Do not instantiate var decls which are not visible.

[rsmith]

I expect this patch to cause problems if the two definitions of the variable template come from different modules, because at deserialization time we don't merge the definitions together sensibly (it looks like we end up with a redeclaration chain with multiple declarations, multiple of which believe they are "the" defintiion).

lib/Sema/SemaTemplate.cpp

470

|| on the previous line, please.

472–476

Variable names should start with a capital letter.

478

We'll need to extend hasVisibleDefinition to handle merged VarDecls to support this. (The ASTReader doesn't currently merge together VarDecl definitions in a reasonable way.)

512–513

else if doesn't make sense here -- we either need to produce a diagnostic on all paths through here, or suppress the notes if we didn't produce a diagnostic.

530

Likewise here.

[v.g.vassilev]

Address comments.

I still find the regression test a bit clumsy. I will try to add it to test/Modules/submodules-merge-defs.cpp, would this be the right place for it?

[rsmith]

I think you'll also need to update the ASTDeclReader to merge VarDecl definitions together if it reads a definition and there already is one in the AST. I note that right now Sema::AddInitializerToDecl permits multiple definitions of a VarDecl, which doesn't seem like what we want here; we'll probably want to merge those as we parse them, too.

Now, we have a problem here: unlike with classes and functions, we can't always convert a variable definition to a declaration by just dropping the initializer. Perhaps we can add a flag to VarDecl to indicate "this is just a declaration, even though it looks like a definition", to handle that case? (This would also be useful for VarTemplateSpecializationDecls, where we currently reject valid code such as "template<typename> int n; int k = n<int>;" because we have no way to represent the difference between a mere declaration and a definition of n<int>.)

lib/Sema/SemaTemplate.cpp

505

Why do we not issue a diagnostic in this case for a VarDecl when Complain is true and no definition is available? It seems like we should either be diagnosing this or asserting that it can't happen.

530

else + assert would make more sense here. No other kind of declaration should get here.

lib/Sema/SemaType.cpp

6904–6905

We should add a getTemplateInstantiationPattern() to VarDecl and use it from here.

[v.g.vassilev]

Address some comments and publish current progress.

[rsmith]

This looks like it's going in the right direction.

lib/AST/Decl.cpp

2269–2272

I think we need a similar check in the static data member case above.

2278

enum?

lib/Sema/SemaTemplate.cpp

512–513

This function still appears to be able to return true (indicating to the caller that a diagnostic was produced) without actually producing a diagnostic.

lib/Serialization/ASTWriterDecl.cpp

896–897

Sink this flag into the "not for ParmVarDecl" block below.

1962

Hmm. The width of the InitStyle field is definitely wrong right now, but should be fixed separately from this change. It looks like we don't hit this today because we don't use this abbreviation for a variable with an initializer. In addition to fixing the width of this field, we should also remove the getInit() == nullptr check when selecting the abbreviation in ASTDeclWriter::VisitVarDecl.

[v.g.vassilev]

Rebase, comments some more fixes.

[v.g.vassilev]

lib/Sema/SemaTemplate.cpp

505

I believe it is not implemented, i.e. we didn't have this diagnostics when VarDecl::getInstantiatedFromStaticDataMember is true. I added a FIXME: for a future enhancement.

512–513

Is it better now?

lib/Serialization/ASTWriterDecl.cpp

896–897

I thought we might need this for c-style void f(struct S arg)-like constructs where we might need to demote if we merge ParmVarDecls.

1962

Committed in r283444.

[rsmith]

lib/Sema/SemaDecl.cpp

3692–3693

We always get here after calling that, don't we? So we only briefly have two definitions, between the point at which we attach the definition and now. If that's what you're referring to here, this comment could be clearer about it.

3695

You should also call makeMergedDefinitionVisible(Def, New->getLocation()) here (and also call it for Def's enclosing template if this is a variable template definition) to make the prior definition visible.

lib/Serialization/ASTReaderDecl.cpp

3083–3087

I don't think this is worthwhile, since it's the first thing the loop below will check.

3089

You should start at PrevVD not at D->First. This loop will currently only iterate once.

3090

You can also bail out early and demote the current definition if you reach a previous demoted definition. That would reduce this from quadratic-time to linear-time.

lib/Serialization/ASTWriterDecl.cpp

896–897

We'll still have only one ParmVarDecl per FunctionDecl, and no-one cares whether a ParmVarDecl is a definition. Also, you assert that the flag is always false in this case below.

[v.g.vassilev]

Address comments.

[rsmith]

include/clang/AST/Decl.h

1222

You can remove this; it's covered by the next line.

lib/AST/Decl.cpp

2284

Missing a member specialization check here.

lib/Serialization/ASTReaderDecl.cpp

3086–3090

Maybe combine these two ifs into one, since their bodies are identical?

[v.g.vassilev]

Landed in r283882.

include/clang/AST/Decl.h

1222

Ok. This would allow calling the method on already demoted definition. Do we want to allow that?

lib/AST/Decl.cpp

2284

Isn't that covered by the cases above? It seems there is no API to make such check, here. The current state looks to me consistent with CXXRecordDecl::getTemplateInstantiationPattern and FunctionDecl::getTemplateInstantiationPattern.

lib/Serialization/ASTReaderDecl.cpp

3086–3090

Good point ;)

[rsmith]

include/clang/AST/Decl.h

1213

This is the bug. You can't read this bit here without first checking whether you are a ParmVarDecl, because ParmVarDecls don't have this bit at all.

1222

Demoted definitions return false from isThisDeclarationADefinition(), so the next line catches this case.

[v.g.vassilev]

We should not access the IsThisDeclarationADemotedDefinition bits if the decl is ParmVarDecl.

[v.g.vassilev]

Relanded in r284008.

[rsmith]

I reverted this in r284081, and relanded with fixes described here as r284284.

lib/Sema/SemaDecl.cpp

9712

We also need to do this work when MergeVarDecls encounters the same condition.

lib/Serialization/ASTReaderDecl.cpp

3087

When we do this, we need to tell the ASTContext we merged the definitions, so that the old definition will be made visible whenever the new one is.

include/clang/AST/Decl.h

protected:
   };
 
   class NonParmVarDeclBitfields {
     friend class VarDecl;
     friend class ASTDeclReader;
 
     unsigned : NumVarDeclBits;
 
+    // FIXME: We need something similar to CXXRecordDecl::DefinitionData.
+    /// \brief Whether this variable is a definition which was demoted due to
+    /// module merge.
+    unsigned IsThisDeclarationADemotedDefinition : 1;
+
     /// \brief Whether this variable is the exception variable in a C++ catch
     /// or an Objective-C @catch statement.
     unsigned ExceptionVar : 1;
 
     /// \brief Whether this local variable could be allocated in the return
     /// slot of its function, enabling the named return value optimization
     /// (NRVO).
     unsigned NRVOVariable : 1;
public:
   /// expression for class types. List-style initialization is C++11 syntax,
   /// e.g. "int x{1};". Clients can distinguish between different forms of
   /// initialization by checking this value. In particular, "int x = {1};" is
   /// C-style, "int x({1})" is call-style, and "int x{1};" is list-style; the
   /// Init expression in all three cases is an InitListExpr.
   InitializationStyle getInitStyle() const {
     return static_cast<InitializationStyle>(VarDeclBits.InitStyle);
   }
-
   /// \brief Whether the initializer is a direct-initializer (list or call).
   bool isDirectInit() const {
     return getInitStyle() != CInit;
   }
 
+  /// \brief If this definition should pretend to be a declaration.
+  bool isThisDeclarationADemotedDefinition() const {
+    return isa<ParmVarDecl>(this) : false ?
+      NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
+  }
+
+  /// \brief This is a definition which should be demoted to a declaration.
+  ///
+  /// In some cases (mostly module merging) we can end up with two visible
+  /// definitions one of which needs to be demoted to a declaration to keep
+  /// the AST invariants.
+  void demoteThisDefinitionToDeclaration() {
+    assert (isThisDeclarationADefinition() && "Not a definition!");
+    assert (!isa<ParmVarDecl>(this) && "Cannot demote ParmVarDecls!");
+    NonParmVarDeclBits.IsThisDeclarationADemotedDefinition = 1;
+  }
+
   /// \brief Determine whether this variable is the exception variable in a
   /// C++ catch statememt or an Objective-C \@catch statement.
   bool isExceptionVariable() const {
     return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.ExceptionVar;
   }
   void setExceptionVariable(bool EV) {
     assert(!isa<ParmVarDecl>(this));
     NonParmVarDeclBits.ExceptionVar = EV;
return isa<ParmVarDecl>(this)
                ? false
                : NonParmVarDeclBits.PreviousDeclInSameBlockScope;
   }
   void setPreviousDeclInSameBlockScope(bool Same) {
     assert(!isa<ParmVarDecl>(this));
     NonParmVarDeclBits.PreviousDeclInSameBlockScope = Same;
   }
 
+  /// \brief Retrieve the variable declaration from which this variable could
+  /// be instantiated, if it is an instantiation (rather than a non-template).
+  VarDecl *getTemplateInstantiationPattern() const;
+
   /// \brief If this variable is an instantiated static data member of a
   /// class template specialization, returns the templated static data member
   /// from which it was instantiated.
   VarDecl *getInstantiatedFromStaticDataMember() const;
 
   /// \brief If this variable is an instantiation of a variable template or a
   /// static data member of a class template, determine what kind of
   /// template specialization or instantiation this is.

lib/AST/Decl.cpp

VarDecl::isThisDeclarationADefinition(ASTContext &C) const {
   //   was defined within the class with the constexpr specifier [...],
   // C++1y [temp.expl.spec]p15:
   //   An explicit specialization of a static data member or an explicit
   //   specialization of a static data member template is a definition if the
   //   declaration includes an initializer; otherwise, it is a declaration.
   //
   // FIXME: How do you declare (but not define) a partial specialization of
   // a static data member template outside the containing class?
+  if (isThisDeclarationADemotedDefinition())
+    return DeclarationOnly;
+
   if (isStaticDataMember()) {
     if (isOutOfLine() &&
         !(getCanonicalDecl()->isInline() &&
           getCanonicalDecl()->isConstexpr()) &&
         (hasInit() ||
          // If the first declaration is out-of-line, this may be an
          // instantiation of an out-of-line partial specialization of a variable
          // template for which we have not yet instantiated the initializer.
bool VarDecl::checkInitIsICE() const {
   Eval->CheckingICE = true;
 
   Eval->IsICE = Init->isIntegerConstantExpr(getASTContext());
   Eval->CheckingICE = false;
   Eval->CheckedICE = true;
   return Eval->IsICE;
 }
 
+VarDecl *VarDecl::getTemplateInstantiationPattern() const {
+  // If it's a variable template specialization, find the template or partial
+  // specialization from which it was instantiated.
+  if (auto *VDTemplSpec = dyn_cast<VarTemplateSpecializationDecl>(this)) {
+    auto From = VDTemplSpec->getInstantiatedFrom();
+    if (auto *VTD = From.dyn_cast<VarTemplateDecl *>()) {
+      while (auto *NewVTD = VTD->getInstantiatedFromMemberTemplate()) {
+        if (NewVTD->isMemberSpecialization())
+          break;
+        VTD = NewVTD;
+      }
+      return VTD->getTemplatedDecl()->getDefinition();
+    }
+    if (auto *VTPSD =
+            From.dyn_cast<VarTemplatePartialSpecializationDecl *>()) {
+      while (auto *NewVTPSD = VTPSD->getInstantiatedFromMember()) {
+        if (NewVTPSD->isMemberSpecialization())
+          break;
+        VTPSD = NewVTPSD;
+      }
+      return VTPSD->getDefinition();
+    }
+  }
+
+  if (MemberSpecializationInfo *MSInfo = getMemberSpecializationInfo()) {
+    if (isTemplateInstantiation(MSInfo->getTemplateSpecializationKind())) {
+      VarDecl *VD = getInstantiatedFromStaticDataMember();
+      while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+        VD = NewVD;
+      return VD->getDefinition();
+    }
+  }
+
+  if (VarTemplateDecl *VarTemplate = getDescribedVarTemplate()) {
+
+    while (VarTemplate->getInstantiatedFromMemberTemplate()) {
+      if (VarTemplate->isMemberSpecialization())
+        break;
+      VarTemplate = VarTemplate->getInstantiatedFromMemberTemplate();
+    }
+
+    assert((!VarTemplate->getTemplatedDecl() ||
+            !isTemplateInstantiation(getTemplateSpecializationKind())) &&
+           "couldn't find pattern for variable instantiation");
+
+    return VarTemplate->getTemplatedDecl();
+  }
+  return nullptr;
+}
+
 VarDecl *VarDecl::getInstantiatedFromStaticDataMember() const {
   if (MemberSpecializationInfo *MSI = getMemberSpecializationInfo())
     return cast<VarDecl>(MSI->getInstantiatedFrom());
 
   return nullptr;
 }
 
 TemplateSpecializationKind VarDecl::getTemplateSpecializationKind() const {

lib/Sema/SemaDecl.cpp

if (!hasVisibleDefinition(Def, &Hidden) &&
         (New->getFormalLinkage() == InternalLinkage ||
          New->getDescribedVarTemplate() ||
          New->getNumTemplateParameterLists() ||
          New->getDeclContext()->isDependentContext())) {
       // The previous definition is hidden, and multiple definitions are
       // permitted (in separate TUs). Form another definition of it.
     } else if (Old->isStaticDataMember() &&
                Old->getCanonicalDecl()->isInline() &&
                Old->getCanonicalDecl()->isConstexpr()) {
       // This definition won't be a definition any more once it's been merged.
       Diag(New->getLocation(),
            diag::warn_deprecated_redundant_constexpr_static_def);
     } else {
       Diag(New->getLocation(), diag::err_redefinition) << New;
       Diag(Def->getLocation(), diag::note_previous_definition);
       New->setInvalidDecl();
       return;
     }
   }
 
if ((Def = VDecl->getDefinition()) && Def != VDecl &&
     NamedDecl *Hidden = nullptr;
     if (!hasVisibleDefinition(Def, &Hidden) &&
         (VDecl->getFormalLinkage() == InternalLinkage ||
          VDecl->getDescribedVarTemplate() ||
          VDecl->getNumTemplateParameterLists() ||
          VDecl->getDeclContext()->isDependentContext())) {
       // The previous definition is hidden, and multiple definitions are
       // permitted (in separate TUs). Form another definition of it.
+
+      // Demote the newly parsed definition to a fake declaration.
+      if (!VDecl->isThisDeclarationADemotedDefinition())
+        VDecl->demoteThisDefinitionToDeclaration();
+
+      // Make the definition visible from the point of the demotion on.
+      assert (!Hidden || Def == Hidden &&
+              "We were suggested another hidden definition!");
+      makeMergedDefinitionVisible(Def, VDecl->getLocation());
+
+      // If this is a variable template definition, make its enclosing template
+      // visible.
+      if (VarDecl *VarPattern = Def->getTemplateInstantiationPattern())
+        if (VarPattern->isThisDeclarationADefinition())
+          makeMergedDefinitionVisible(VarPattern, VDecl->getLocation());
     } else {
       Diag(VDecl->getLocation(), diag::err_redefinition)
         << VDecl->getDeclName();
       Diag(Def->getLocation(), diag::note_previous_definition);
       VDecl->setInvalidDecl();
       return;
     }
   }

lib/Sema/SemaTemplate.cpp

 /// it either has no definition, or is in the process of being instantiated).
 bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
                                           NamedDecl *Instantiation,
                                           bool InstantiatedFromMember,
                                           const NamedDecl *Pattern,
                                           const NamedDecl *PatternDef,
                                           TemplateSpecializationKind TSK,
                                           bool Complain /*= true*/) {
-  assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation));
+  assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation) ||
+         isa<VarDecl>(Instantiation));
 
-  if (PatternDef && (isa<FunctionDecl>(PatternDef)
-                     || !cast<TagDecl>(PatternDef)->isBeingDefined())) {
+  bool IsEntityBeingDefined = false;
+  if (const TagDecl *TD = dyn_cast_or_null<TagDecl>(PatternDef))
+    IsEntityBeingDefined = TD->isBeingDefined();
+
+  if (PatternDef && !IsEntityBeingDefined) {
     NamedDecl *SuggestedDef = nullptr;
     if (!hasVisibleDefinition(const_cast<NamedDecl*>(PatternDef), &SuggestedDef,
                               /*OnlyNeedComplete*/false)) {
       // If we're allowed to diagnose this and recover, do so.
       bool Recover = Complain && !isSFINAEContext();
       if (Complain)
         diagnoseMissingImport(PointOfInstantiation, SuggestedDef,
                               Sema::MissingImportKind::Definition, Recover);
       return !Recover;
     }
     return false;
   }
 
   if (!Complain || (PatternDef && PatternDef->isInvalidDecl()))
     return true;
 
+  llvm::Optional<unsigned> Note;
   QualType InstantiationTy;
   if (TagDecl *TD = dyn_cast<TagDecl>(Instantiation))
     InstantiationTy = Context.getTypeDeclType(TD);
   if (PatternDef) {
     Diag(PointOfInstantiation,
          diag::err_template_instantiate_within_definition)
-      << (TSK != TSK_ImplicitInstantiation)
+      << /*implicit|explicit*/(TSK != TSK_ImplicitInstantiation)
       << InstantiationTy;
     // Not much point in noting the template declaration here, since
     // we're lexically inside it.
     Instantiation->setInvalidDecl();
   } else if (InstantiatedFromMember) {
     if (isa<FunctionDecl>(Instantiation)) {
       Diag(PointOfInstantiation,
            diag::err_explicit_instantiation_undefined_member)
-        << 1 << Instantiation->getDeclName() << Instantiation->getDeclContext();
+        << /*member function*/ 1 << Instantiation->getDeclName()
+        << Instantiation->getDeclContext();
+      Note = diag::note_explicit_instantiation_here;
     } else {
+      assert(isa<TagDecl>(Instantiation) && "Must be a TagDecl!");
       Diag(PointOfInstantiation,
            diag::err_implicit_instantiate_member_undefined)
         << InstantiationTy;
+      Note = diag::note_member_declared_at;
     }
-    Diag(Pattern->getLocation(), isa<FunctionDecl>(Instantiation)
-                                     ? diag::note_explicit_instantiation_here
-                                     : diag::note_member_declared_at);
   } else {
-    if (isa<FunctionDecl>(Instantiation))
+    if (isa<FunctionDecl>(Instantiation)) {
       Diag(PointOfInstantiation,
            diag::err_explicit_instantiation_undefined_func_template)
         << Pattern;
-    else
+      Note = diag::note_explicit_instantiation_here;
+    } else if (isa<TagDecl>(Instantiation)) {
       Diag(PointOfInstantiation, diag::err_template_instantiate_undefined)
         << (TSK != TSK_ImplicitInstantiation)
         << InstantiationTy;
-    Diag(Pattern->getLocation(), isa<FunctionDecl>(Instantiation)
-                                     ? diag::note_explicit_instantiation_here
-                                     : diag::note_template_decl_here);
+      Note = diag::note_template_decl_here;
+    } else {
+      assert(isa<VarDecl>(Instantiation) && "Must be a VarDecl!");
+      if (isa<VarTemplateSpecializationDecl>(Instantiation)) {
+        Diag(PointOfInstantiation,
+             diag::err_explicit_instantiation_undefined_var_template)
+          << Instantiation;
+        Instantiation->setInvalidDecl();
+      } else
+        Diag(PointOfInstantiation,
+             diag::err_explicit_instantiation_undefined_member)
+          << /*static data member*/ 2 << Instantiation->getDeclName()
+          << Instantiation->getDeclContext();
+      Note = diag::note_explicit_instantiation_here;
+    }
   }
+  if (Note) // Diagnostics were emitted.
+    Diag(Pattern->getLocation(), Note.getValue());
 
   // In general, Instantiation isn't marked invalid to get more than one
   // error for multiple undefined instantiations. But the code that does
   // explicit declaration -> explicit definition conversion can't handle
   // invalid declarations, so mark as invalid in that case.
   if (TSK == TSK_ExplicitInstantiationDeclaration)
     Instantiation->setInvalidDecl();
   return true;

lib/Sema/SemaTemplateInstantiateDecl.cpp

if (PatternDecl->isStaticDataMember() &&
       // FIXME: Factor out the duplicated instantiation context setup/tear down
       // code here.
       InstantiatingTemplate Inst(*this, PointOfInstantiation, Var);
       if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
         return;
       PrettyDeclStackTraceEntry CrashInfo(*this, Var, SourceLocation(),
                                           "instantiating variable initializer");
 
+      // The instantiation is visible here, even if it was first declared in an
+      // unimported module.
+      Var->setHidden(false);
+
       // If we're performing recursive template instantiation, create our own
       // queue of pending implicit instantiations that we will instantiate
       // later, while we're still within our own instantiation context.
       SavePendingInstantiationsAndVTableUsesRAII
           SavePendingInstantiationsAndVTableUses(*this, /*Enabled=*/Recursive);
 
       LocalInstantiationScope Local(*this);
 
if (VarSpec) {
     assert(Var->isStaticDataMember() && "not a static data member?");
     PatternDecl = Var->getInstantiatedFromStaticDataMember();
 
     assert(PatternDecl && "data member was not instantiated from a template?");
     assert(PatternDecl->isStaticDataMember() && "not a static data member?");
     Def = PatternDecl->getDefinition();
   }
 
-  // FIXME: Check that the definition is visible before trying to instantiate
-  // it. This requires us to track the instantiation stack in order to know
-  // which definitions should be visible.
+  TemplateSpecializationKind TSK = Var->getTemplateSpecializationKind();
 
   // If we don't have a definition of the variable template, we won't perform
   // any instantiation. Rather, we rely on the user to instantiate this
   // definition (or provide a specialization for it) in another translation
   // unit.
-  if (!Def) {
-    if (DefinitionRequired) {
-      if (VarSpec) {
-        Diag(PointOfInstantiation,
-             diag::err_explicit_instantiation_undefined_var_template) << Var;
-        Var->setInvalidDecl();
-      }
-      else
-        Diag(PointOfInstantiation,
-             diag::err_explicit_instantiation_undefined_member)
-            << 2 << Var->getDeclName() << Var->getDeclContext();
-      Diag(PatternDecl->getLocation(),
-           diag::note_explicit_instantiation_here);
-    } else if (Var->getTemplateSpecializationKind()
-                 == TSK_ExplicitInstantiationDefinition) {
+  if (!Def && !DefinitionRequired) {
+    if (TSK == TSK_ExplicitInstantiationDefinition) {
       PendingInstantiations.push_back(
         std::make_pair(Var, PointOfInstantiation));
-    } else if (Var->getTemplateSpecializationKind()
-                 == TSK_ImplicitInstantiation) {
+    } else if (TSK == TSK_ImplicitInstantiation) {
       // Warn about missing definition at the end of translation unit.
       if (AtEndOfTU && !getDiagnostics().hasErrorOccurred()) {
         Diag(PointOfInstantiation, diag::warn_var_template_missing)
           << Var;
         Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
         if (getLangOpts().CPlusPlus11)
           Diag(PointOfInstantiation, diag::note_inst_declaration_hint) << Var;
       }
+      return;
     }
 
-    return;
   }
 
-  TemplateSpecializationKind TSK = Var->getTemplateSpecializationKind();
+  // FIXME: We need to track the instantiation stack in order to know which
+  // definitions should be visible within this instantiation.
+  // FIXME: Produce diagnostics when Var->getInstantiatedFromStaticDataMember().
+  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Var,
+                                     /*InstantiatedFromMember*/false,
+                                     PatternDecl, Def, TSK,
+                                     /*Complain*/DefinitionRequired))
+    return;
+
 
   // Never instantiate an explicit specialization.
   if (TSK == TSK_ExplicitSpecialization)
     return;
 
   // C++11 [temp.explicit]p10:
   //   Except for inline functions, [...] explicit instantiation declarations
   //   have the effect of suppressing the implicit instantiation of the entity

lib/Sema/SemaType.cpp

if (OnlyNeedComplete && ED->isFixed()) {
       }
       return false;
     }
     D = ED->getDefinition();
   } else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
     if (auto *Pattern = FD->getTemplateInstantiationPattern())
       FD = Pattern;
     D = FD->getDefinition();
+  } else if (auto *VD = dyn_cast<VarDecl>(D)) {
+    if (auto *Pattern = VD->getTemplateInstantiationPattern())
+      VD = Pattern;
+    D = VD->getDefinition();
   }
   assert(D && "missing definition for pattern of instantiated definition");
 
   *Suggested = D;
   if (isVisible(D))
     return true;
 
   // The external source may have additional definitions of this entity that are

lib/Serialization/ASTReaderDecl.cpp

 ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
   RedeclarableResult Redecl = VisitRedeclarable(VD);
   VisitDeclaratorDecl(VD);
 
   VD->VarDeclBits.SClass = (StorageClass)Record[Idx++];
   VD->VarDeclBits.TSCSpec = Record[Idx++];
   VD->VarDeclBits.InitStyle = Record[Idx++];
   if (!isa<ParmVarDecl>(VD)) {
+    VD->NonParmVarDeclBits.IsThisDeclarationADemotedDefinition = Record[Idx++];
     VD->NonParmVarDeclBits.ExceptionVar = Record[Idx++];
     VD->NonParmVarDeclBits.NRVOVariable = Record[Idx++];
     VD->NonParmVarDeclBits.CXXForRangeDecl = Record[Idx++];
     VD->NonParmVarDeclBits.ARCPseudoStrong = Record[Idx++];
     VD->NonParmVarDeclBits.IsInline = Record[Idx++];
     VD->NonParmVarDeclBits.IsInlineSpecified = Record[Idx++];
     VD->NonParmVarDeclBits.IsConstexpr = Record[Idx++];
     VD->NonParmVarDeclBits.IsInitCapture = Record[Idx++];
void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
                                            Decl *Previous, Decl *Canon) {
   D->RedeclLink.setPrevious(cast<DeclT>(Previous));
   D->First = cast<DeclT>(Previous)->First;
 }
 
 namespace clang {
 template<>
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
+                                           Redeclarable<VarDecl> *D,
+                                           Decl *Previous, Decl *Canon) {
+  VarDecl *VD = static_cast<VarDecl*>(D);
+  VarDecl *PrevVD = cast<VarDecl>(Previous);
+  D->RedeclLink.setPrevious(PrevVD);
+  D->First = PrevVD->First;
+
+  // We should keep at most one definition on the chain.
+  if (VD->isThisDeclarationADefinition()) {
+    for (VarDecl *CurD = PrevVD; CurD; CurD = CurD->getPreviousDecl()) {
+      // If we find an already demoted definition, this we already visited this
+      // part of the chain. Reduces the loop from quadratic-time to linear-time.
+      if (CurD->isThisDeclarationADemotedDefinition() ||
+          CurD->isThisDeclarationADefinition()) {
+        VD->demoteThisDefinitionToDeclaration();
+        break;
+      }
+    }
+  }
+}
+
+template<>
+void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
                                            Redeclarable<FunctionDecl> *D,
                                            Decl *Previous, Decl *Canon) {
   FunctionDecl *FD = static_cast<FunctionDecl*>(D);
   FunctionDecl *PrevFD = cast<FunctionDecl>(Previous);
 
   FD->RedeclLink.setPrevious(PrevFD);
   FD->First = PrevFD->First;
 

lib/Serialization/ASTWriterDecl.cpp

 }
 
 void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
   VisitRedeclarable(D);
   VisitDeclaratorDecl(D);
   Record.push_back(D->getStorageClass());
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
   if (!isa<ParmVarDecl>(D)) {
+    Record.push_back(D->isThisDeclarationADemotedDefinition());
     Record.push_back(D->isExceptionVariable());
     Record.push_back(D->isNRVOVariable());
     Record.push_back(D->isCXXForRangeDecl());
     Record.push_back(D->isARCPseudoStrong());
     Record.push_back(D->isInline());
     Record.push_back(D->isInlineSpecified());
     Record.push_back(D->isConstexpr());
     Record.push_back(D->isInitCapture());
if (D->getDeclContext() == D->getLexicalDeclContext() &&
       !D->hasInheritedDefaultArg() &&
       D->getInit() == nullptr &&
       !D->hasUninstantiatedDefaultArg())  // No default expr.
     AbbrevToUse = Writer.getDeclParmVarAbbrev();
 
   // Check things we know are true of *every* PARM_VAR_DECL, which is more than
   // just us assuming it.
   assert(!D->getTSCSpec() && "PARM_VAR_DECL can't use TLS");
+  assert(!D->isThisDeclarationADemotedDefinition()
+         && "PARM_VAR_DECL can't be demoted definition.");
   assert(D->getAccess() == AS_none && "PARM_VAR_DECL can't be public/private");
   assert(!D->isExceptionVariable() && "PARM_VAR_DECL can't be exception var");
   assert(D->getPreviousDecl() == nullptr && "PARM_VAR_DECL can't be redecl");
   assert(!D->isStaticDataMember() &&
          "PARM_VAR_DECL can't be static data member");
 }
 
 void ASTDeclWriter::VisitDecompositionDecl(DecompositionDecl *D) {
void ASTWriter::WriteDeclAbbrevs() {
   // ValueDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Type
   // DeclaratorDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // InnerStartLoc
   Abv->Add(BitCodeAbbrevOp(0));                       // hasExtInfo
   // VarDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // SClass
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));                         // isInline
   Abv->Add(BitCodeAbbrevOp(0));                         // isInlineSpecified
   Abv->Add(BitCodeAbbrevOp(0));                         // isConstexpr
   Abv->Add(BitCodeAbbrevOp(0));                         // isInitCapture

test/Modules/Inputs/PR28752/Subdir1/b.h

+#include <vector>

test/Modules/Inputs/PR28752/Subdir1/module.modulemap

+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}

test/Modules/Inputs/PR28752/a.h

+#include <vector>

test/Modules/Inputs/PR28752/module.modulemap

+module a { header "a.h" export * }

test/Modules/Inputs/PR28752/vector

+#ifndef VECTOR
+#define VECTOR
+template <bool, typename> struct B;
+template <typename _Tp> struct B<true, _Tp> { typedef _Tp type; };
+namespace std {
+template <typename> struct D {
+
+  template <typename _Alloc2> struct F {
+    static const bool value = 0;
+  };
+
+  template <typename _Alloc2>
+  typename B<F<_Alloc2>::value, _Alloc2>::type _S_select(_Alloc2);
+  template <typename _Alloc2>
+  static
+  typename B<!F<_Alloc2>::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template <typename _Alloc>
+template <typename _Alloc2>
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template <typename> class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D<bool>::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
 No newline at end of file

test/Modules/pr28752.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include <vector>
+
+class TClingClassInfo {
+  std::vector<int> fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+