[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added a comment.

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




Comment at: lib/Sema/SemaDecl.cpp:9712
+
+  // Demote the newly parsed definition to a fake declaration.
+  if (!VDecl->isThisDeclarationADemotedDefinition())

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



Comment at: lib/Serialization/ASTReaderDecl.cpp:3087
+  CurD->isThisDeclarationADefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;

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.


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev closed this revision.
v.g.vassilev marked an inline comment as done.
v.g.vassilev added a comment.

Relanded in r284008.


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev updated this revision to Diff 74353.
v.g.vassilev marked 2 inline comments as done.
v.g.vassilev added a comment.

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


https://reviews.llvm.org/D24508

Files:
  include/clang/AST/Decl.h
  lib/AST/Decl.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template 
+template 
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template  class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include 
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include 
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -894,6 +894,7 @@
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
   if (!isa(D)) {
+Record.push_back(D->isThisDeclarationADemotedDefinition());
 Record.push_back(D->isExceptionVariable());
 Record.push_back(D->isNRVOVariable());
 Record.push_back(D->isCXXForRangeDecl());
@@ -998,6 +999,8 @@
   // 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");
@@ -1957,6 +1960,7 @@
   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
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/Decl.h:1213
+  bool isThisDeclarationADemotedDefinition() const {
+return NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
+  }

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.



Comment at: include/clang/AST/Decl.h:1222
+  void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");

v.g.vassilev wrote:
> rsmith wrote:
> > You can remove this; it's covered by the next line.
> Ok. This would allow calling the method on already demoted definition. Do we 
> want to allow that?
Demoted definitions return false from `isThisDeclarationADefinition()`, so the 
next line catches this case.


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev marked 2 inline comments as done.
v.g.vassilev added a comment.

Landed in r283882.




Comment at: include/clang/AST/Decl.h:1222
+  void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");

rsmith wrote:
> You can remove this; it's covered by the next line.
Ok. This would allow calling the method on already demoted definition. Do we 
want to allow that?



Comment at: lib/AST/Decl.cpp:2284
+  while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+VD = NewVD;
+  return VD->getDefinition();

rsmith wrote:
> Missing a member specialization check here.
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`.



Comment at: lib/Serialization/ASTReaderDecl.cpp:3086-3090
+  if (CurD->isThisDeclarationADemotedDefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;
+  }
+  if (CurD->isThisDeclarationADefinition()) {

rsmith wrote:
> Maybe combine these two `if`s into one, since their bodies are identical?
Good point ;)


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: include/clang/AST/Decl.h:1222
+  void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");

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



Comment at: lib/AST/Decl.cpp:2284
+  while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+VD = NewVD;
+  return VD->getDefinition();

Missing a member specialization check here.



Comment at: lib/Serialization/ASTReaderDecl.cpp:3086-3090
+  if (CurD->isThisDeclarationADemotedDefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;
+  }
+  if (CurD->isThisDeclarationADefinition()) {

Maybe combine these two `if`s into one, since their bodies are identical?


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev updated this revision to Diff 74129.
v.g.vassilev marked 3 inline comments as done.
v.g.vassilev added a comment.

Address comments.


https://reviews.llvm.org/D24508

Files:
  include/clang/AST/Decl.h
  lib/AST/Decl.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template 
+template 
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template  class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include 
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include 
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -894,6 +894,7 @@
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
   if (!isa(D)) {
+Record.push_back(D->isThisDeclarationADemotedDefinition());
 Record.push_back(D->isExceptionVariable());
 Record.push_back(D->isNRVOVariable());
 Record.push_back(D->isCXXForRangeDecl());
@@ -998,6 +999,8 @@
   // 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");
@@ -1957,6 +1960,7 @@
   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
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -1216,6 +1216,7 

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added inline comments.


> SemaDecl.cpp:3692-3693
> +  // Demote the newly parsed definition to a fake declaration.
> +  // FIXME: Sema::AddInitializationToDecl still allows two definitions,
> +  // which make the AST variants inconsistent.
> +  assert (Def != New && "There is only one definition!");

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.

> SemaDecl.cpp:3695
> +  assert (Def != New && "There is only one definition!");
> +  New->demoteThisDefinitionToDeclaration();
>  } else if (Old->isStaticDataMember() &&

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.

> ASTReaderDecl.cpp:3083-3087
> +// Fast track.
> +if (PrevVD->isThisDeclarationADefinition()) {
> +  VD->demoteThisDefinitionToDeclaration();
> +  return;
> +}

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

> ASTReaderDecl.cpp:3089
> +
> +for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> +  if (CurD->isThisDeclarationADefinition()) {

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

> ASTReaderDecl.cpp:3090
> +for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> +  if (CurD->isThisDeclarationADefinition()) {
> +// If we found another definition on the chain, demote the current 
> one.

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.

> v.g.vassilev wrote in 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.

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.

https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev added inline comments.


> rsmith wrote in SemaTemplate.cpp:509
> This function still appears to be able to return true (indicating to the 
> caller that a diagnostic was produced) without actually producing a 
> diagnostic.

Is it better now?

> rsmith wrote in 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.

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.

> rsmith wrote in ASTWriterDecl.cpp:896-897
> Sink this flag into the "not for `ParmVarDecl`" block below.

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.

> rsmith wrote in ASTWriterDecl.cpp:1965
> 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`.

Committed in r283444.

https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev updated this revision to Diff 73803.
v.g.vassilev marked 4 inline comments as done.
v.g.vassilev added a comment.

Rebase, comments some more fixes.


https://reviews.llvm.org/D24508

Files:
  include/clang/AST/Decl.h
  lib/AST/Decl.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/Inputs/merge-class-definition-visibility/a.h
  test/Modules/Inputs/merge-class-definition-visibility/b.h
  test/Modules/Inputs/merge-class-definition-visibility/c.h
  test/Modules/Inputs/merge-class-definition-visibility/d.h
  test/Modules/Inputs/merge-class-definition-visibility/e.h
  test/Modules/merge-class-definition-visibility.cpp
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/merge-class-definition-visibility.cpp
===
--- test/Modules/merge-class-definition-visibility.cpp
+++ test/Modules/merge-class-definition-visibility.cpp
@@ -1,6 +1,6 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/merge-class-definition-visibility/modmap \
-// RUN:-I%S/Inputs/merge-class-definition-visibility \
+// RUN:-std=c++1z -I%S/Inputs/merge-class-definition-visibility \
 // RUN:-fmodules-cache-path=%t %s -verify \
 // RUN:-fmodules-local-submodule-visibility
 // expected-no-diagnostics
@@ -10,6 +10,9 @@
 typedef X XA;
 struct B;
 
+template int n;
+
+
 #include "e.h"
 // Ensure that this triggers the import of the second definition from e.h,
 // which is necessary to make the definition of A visible in the template
@@ -21,3 +24,5 @@
 // and that definition was merged into the canonical definition from b.h,
 // so that becomes visible, and we have a visible definition.
 B b;
+
+void f (int dflt = n) {}
Index: test/Modules/Inputs/merge-class-definition-visibility/e.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/e.h
+++ test/Modules/Inputs/merge-class-definition-visibility/e.h
@@ -1,3 +1,5 @@
 #include "a.h"
 
 struct B {};
+
+template int n;
Index: test/Modules/Inputs/merge-class-definition-visibility/d.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/d.h
+++ test/Modules/Inputs/merge-class-definition-visibility/d.h
@@ -1 +1,3 @@
 struct B {};
+
+template int n;
Index: test/Modules/Inputs/merge-class-definition-visibility/c.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/c.h
+++ test/Modules/Inputs/merge-class-definition-visibility/c.h
@@ -1 +1,3 @@
 struct A;
+
+inline int var_A;
Index: test/Modules/Inputs/merge-class-definition-visibility/b.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/b.h
+++ test/Modules/Inputs/merge-class-definition-visibility/b.h
@@ -2,3 +2,6 @@
 #include "a.h"
 
 struct B {};
+
+template int n;
+int k = n;
Index: test/Modules/Inputs/merge-class-definition-visibility/a.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/a.h
+++ test/Modules/Inputs/merge-class-definition-visibility/a.h
@@ -1 +1,3 @@
 struct A {};
+
+inline int var_A = 2;
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B:

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added a comment.

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



> Decl.cpp:2269-2272
> +  // If we have hit a point where the user provided a specialization of
> +  // this template, we're done looking.
> +  if (VarTemplate->isMemberSpecialization())
> +break;

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

> Decl.cpp:2278
> +!isTemplateInstantiation(getTemplateSpecializationKind())) &&
> +   "couldn't find pattern for enum instantiation");
> +

enum?

> rsmith wrote in SemaTemplate.cpp:509
> `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.

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

> ASTWriterDecl.cpp:896-897
>Record.push_back(D->getInitStyle());
> +  Record.push_back(D->isThisDeclarationADemotedDefinition());
>if (!isa(D)) {
>  Record.push_back(D->isExceptionVariable());

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

> ASTWriterDecl.cpp:1965
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // 
> IsThisDeclarationADemotedDefinition

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`.

https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev updated this revision to Diff 73688.
v.g.vassilev added a comment.

Address some comments and publish current progress.


https://reviews.llvm.org/D24508

Files:
  include/clang/AST/Decl.h
  lib/AST/Decl.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/Inputs/merge-class-definition-visibility/a.h
  test/Modules/Inputs/merge-class-definition-visibility/c.h
  test/Modules/merge-class-definition-visibility.cpp
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/merge-class-definition-visibility.cpp
===
--- test/Modules/merge-class-definition-visibility.cpp
+++ test/Modules/merge-class-definition-visibility.cpp
@@ -1,6 +1,6 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/merge-class-definition-visibility/modmap \
-// RUN:-I%S/Inputs/merge-class-definition-visibility \
+// RUN:-std=c++1z -I%S/Inputs/merge-class-definition-visibility \
 // RUN:-fmodules-cache-path=%t %s -verify \
 // RUN:-fmodules-local-submodule-visibility
 // expected-no-diagnostics
Index: test/Modules/Inputs/merge-class-definition-visibility/c.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/c.h
+++ test/Modules/Inputs/merge-class-definition-visibility/c.h
@@ -1 +1,3 @@
 struct A;
+
+inline int var_A;
Index: test/Modules/Inputs/merge-class-definition-visibility/a.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/a.h
+++ test/Modules/Inputs/merge-class-definition-visibility/a.h
@@ -1 +1,3 @@
 struct A {};
+
+inline int var_A = 2;
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template 
+template 
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template  class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include 
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include 
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -893,6 +893,7 @@
   Record.push_back(D->getStorageClass());
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
+  Record.push_back(D->isThisDeclarationADemotedDefinition());
   if (!isa(D))

Re: [PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added a comment.

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 `VarTemplateSpecializationDecl`s, where we 
currently reject valid code such as "template int n; int k = n;" 
because we have no way to represent the difference between a mere declaration 
and a definition of `n`.)



Comment at: lib/Sema/SemaTemplate.cpp:505
@@ -499,3 +504,3 @@
 Instantiation->setInvalidDecl();
   } else if (InstantiatedFromMember) {
 if (isa(Instantiation)) {

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.


Comment at: lib/Sema/SemaTemplate.cpp:528
@@ +527,3 @@
+  Note = diag::note_template_decl_here;
+} else if (isa(Instantiation)) {
+  if (isa(Instantiation)) {

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


Comment at: lib/Sema/SemaType.cpp:6898-6899
@@ +6897,4 @@
+  } else if (auto *VD = dyn_cast(D)) {
+// FIXME: Handle the case where D is a VarTemplateSpecializationDecl, i.e. 
D
+// is not a static data member.
+if (auto *Pattern = VD->getInstantiatedFromStaticDataMember())

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


https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev removed rL LLVM as the repository for this revision.
v.g.vassilev updated this revision to Diff 71317.
v.g.vassilev marked 4 inline comments as done.
v.g.vassilev added a comment.

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?


https://reviews.llvm.org/D24508

Files:
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template 
+template 
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template  class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include 
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include 
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -6894,6 +6894,12 @@
 if (auto *Pattern = FD->getTemplateInstantiationPattern())
   FD = Pattern;
 D = FD->getDefinition();
+  } else if (auto *VD = dyn_cast(D)) {
+// FIXME: Handle the case where D is a VarTemplateSpecializationDecl, i.e. D
+// is not a static data member.
+if (auto *Pattern = VD->getInstantiatedFromStaticDataMember())
+  VD = Pattern;
+D = VD->getDefinition();
   }
   assert(D && "missing definition for pattern of instantiated definition");
 
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4067,6 +4067,10 @@
   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.
@@ -4115,47 +4119,38 @@
 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(

Re: [PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added a comment.

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).



Comment at: lib/Sema/SemaTemplate.cpp:470
@@ -470,1 +469,3 @@
+  assert(isa(Instantiation) || isa(Instantiation)
+ || isa(Instantiation));
 

`||` on the previous line, please.


Comment at: lib/Sema/SemaTemplate.cpp:472
@@ -470,3 +471,3 @@
 
-  if (PatternDef && (isa(PatternDef)
- || !cast(PatternDef)->isBeingDefined())) {
+  bool isEntityBeingDefined = false;
+  if (const TagDecl *TD = dyn_cast_or_null(PatternDef))

Variable names should start with a capital letter.


Comment at: lib/Sema/SemaTemplate.cpp:478
@@ -473,3 +477,3 @@
 NamedDecl *SuggestedDef = nullptr;
 if (!hasVisibleDefinition(const_cast(PatternDef), 
&SuggestedDef,
   /*OnlyNeedComplete*/false)) {

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


Comment at: lib/Sema/SemaTemplate.cpp:509
@@ -504,3 +508,3 @@
 << 1 << Instantiation->getDeclName() << 
Instantiation->getDeclContext();
-} else {
+} else if (isa(Instantiation)) {
   Diag(PointOfInstantiation,

`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.


Comment at: lib/Sema/SemaTemplate.cpp:529
@@ +528,3 @@
+  Note = diag::note_template_decl_here;
+} else if (isa(Instantiation)) {
+  if (isa(Instantiation)) {

Likewise here.


Repository:
  rL LLVM

https://reviews.llvm.org/D24508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev created this revision.
v.g.vassilev added a reviewer: rsmith.
v.g.vassilev added a subscriber: cfe-commits.
v.g.vassilev set the repository for this revision to rL LLVM.

Repository:
  rL LLVM

https://reviews.llvm.org/D24508

Files:
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/Modules/Inputs/PR28752/Subdir1/b.h
  test/Modules/Inputs/PR28752/Subdir1/c.h
  test/Modules/Inputs/PR28752/Subdir1/module.modulemap
  test/Modules/Inputs/PR28752/a.h
  test/Modules/Inputs/PR28752/module.modulemap
  test/Modules/Inputs/PR28752/vector
  test/Modules/pr28752.cpp

Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// 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 
+
+class TClingClassInfo {
+  std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+  TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template  struct B;
+template  struct B { typedef _Tp type; };
+namespace std {
+template  struct D {
+
+  template  struct F {
+static const bool value = 0;
+  };
+
+  template 
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+  template 
+  static
+  typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template 
+template 
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template  class vector {
+public:
+  vector(int);
+  vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include 
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+  module "b.h" { header "b.h" export * }
+  module "c.h" { header "c.h" export * }
+  export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include 
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4067,6 +4067,10 @@
   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.
@@ -4115,47 +4119,38 @@
 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;
-  else
-Diag(PointOfInstantiation,
- diag::err_explicit_instantiation_undefined_member)
-<< 2 << Var->getDeclName() << Var->getDeclContext();
-  Diag(PatternDecl->getLocation(),
-   diag::note_explicit_instantiation_here);
-  if (VarSpec)
-Var->setInvalidDecl();
-} else if (Var->getTemplateSpecializationKind()
- == TSK_Expl