[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added a comment.

I'm really out of my depth in this code, but it looks like that test case is 
triggering the one code path in that function that is actually correct. Could 
you adjust it to trigger the code path behind the first if statement?


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Richard Smith via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/DeclCXX.cpp:1349-1354
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }

loladiro wrote:
> rsmith wrote:
> > The same bug exists in `VarDecl::getTemplateInstantiationPattern`; can you 
> > fix it there too?
> Do you have a test case that shows a problem with the current definition of 
> that function? Would like to include it if possible. I tried cooking one up, 
> but wasn't particularly successful.
Since this is only currently used by the modules visibility code, you'd 
presumably need something like this:
```
  // submodule a.x
  template struct A {
template static const int n;
  };
```
```
  // submodule a.y
  import a.x
  template template const int A::n = 1;
```
```
  // submodule a.z
  import a.x
  template<> template const int A::n = 2;
```
```
  // test
  import a.z
  // I expect this to fail because we would incorrectly check to see whether 
the definition from a.y is visible, because we never check whether the 
definition from a.z is a member specialization.
  static_assert(A::n == 2);


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added inline comments.



Comment at: lib/AST/DeclCXX.cpp:1349-1354
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }

rsmith wrote:
> The same bug exists in `VarDecl::getTemplateInstantiationPattern`; can you 
> fix it there too?
Do you have a test case that shows a problem with the current definition of 
that function? Would like to include it if possible. I tried cooking one up, 
but wasn't particularly successful.


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Richard Smith via cfe-commits]

rsmith added a comment.

Please factor out the fix for `getTemplateInstantiationPattern` and commit it 
separately. https://reviews.llvm.org/D25942 has a testcase for that portion of 
this change.




Comment at: lib/AST/Decl.cpp:1057-1058
+RD->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = RD->getInstantiatedFromMemberClass();
 if (InstantiatedFrom)

This doesn't seem to match GCC; GCC seems to do this for all kinds of member 
specialization, not just for member class specialization:

```
template struct A {
  template struct __attribute__((visibility("hidden"))) B;
};
template<> template struct A::B { virtual void f() {} };
void g() { A::B().f(); }
```

Here, A::B gets hidden visibility, even though it was instantiated 
from the member template specialization `A::B` which had no visibility 
attribute.

I don't know what the underlying logic is here -- does GCC look at the 
visibility on the enclosing class when computing visibility for a member class? 
-- but I don't think this one special case covers it.



Comment at: lib/AST/Decl.cpp:1096-1097
+FunctionDecl *InstantiatedFrom = fn->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = fn->getInstantiatedFromMemberFunction();
 

Likewise.



Comment at: lib/AST/DeclCXX.cpp:1349-1354
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }

The same bug exists in `VarDecl::getTemplateInstantiationPattern`; can you fix 
it there too?


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 76257.
loladiro added a comment.

Add the expected-error annotation


Repository:
  rL LLVM

https://reviews.llvm.org/D13419

Files:
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  test/CodeGenCXX/visibility.cpp
  test/Modules/cxx-templates.cpp

Index: test/Modules/cxx-templates.cpp
===
--- test/Modules/cxx-templates.cpp
+++ test/Modules/cxx-templates.cpp
@@ -199,7 +199,7 @@
 cls uk4; // expected-error 1+{{partial specialization of 'cls' must be imported}} expected-error 1+{{definition of}}
 cls::nested_cls unk1; // expected-error 1+{{explicit specialization of 'nested_cls' must be imported}} expected-error 1+{{definition of}}
 cls::nested_cls_t unk2; // expected-error 1+{{explicit specialization of 'nested_cls_t' must be imported}} expected-error 1+{{definition of}}
-cls::nested_cls_t unk3; // expected-error 1+{{explicit specialization of 'nested_cls_t' must be imported}}
+cls::nested_cls_t unk3; // expected-error 1+{{explicit specialization of 'nested_cls_t' must be imported}} expected-error 1+{{definition of}}
 
 // For enums, uses that would trigger instantiations of definitions are not
 // allowed.
Index: test/CodeGenCXX/visibility.cpp
===
--- test/CodeGenCXX/visibility.cpp
+++ test/CodeGenCXX/visibility.cpp
@@ -1317,3 +1317,59 @@
   // CHECK-LABEL: define void @_ZN6test693foo1fEv
   // CHECK-HIDDEN-LABEL: define hidden void @_ZN6test693foo1fEv
 }
+
+namespace test70 {
+template 
+class foo {
+public:
+  T x;
+  template 
+  HIDDEN S AddS(S);
+  template 
+  class HIDDEN barS {
+  public:
+static S AddS2(foo x, S);
+  };
+  template 
+  class HIDDEN barZ {
+  public:
+template 
+static S AddSZ(foo x, S, Z);
+  };
+};
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+template 
+template 
+HIDDEN S foo::AddS(S y) {
+  return ((S)x) + y;
+}
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+template 
+template 
+HIDDEN S foo::barS::AddS2(foo x, S y) {
+  return ((S)x.x) + y;
+}
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+template 
+template 
+template 
+HIDDEN S foo::barZ::AddSZ(foo x, S y, Z z) {
+  return ((S)x.x) + y + ((S)z);
+}
+
+extern template struct foo;
+template struct foo;
+
+void f() {
+  auto var = foo{5};
+  auto bar = var.AddS((long long)3);
+  auto bar2 = decltype(var)::barS::AddS2(var, 3);
+  auto bar3 = decltype(var)::barZ::AddSZ(var, 3, (char)0);
+}
+}
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1346,17 +1346,19 @@
   if (auto *TD = dyn_cast(this)) {
 auto From = TD->getInstantiatedFrom();
 if (auto *CTD = From.dyn_cast()) {
-  while (auto *NewCTD = CTD->getInstantiatedFromMemberTemplate()) {
-if (NewCTD->isMemberSpecialization())
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }
   return CTD->getTemplatedDecl()->getDefinition();
 }
 if (auto *CTPSD =
 From.dyn_cast()) {
-  while (auto *NewCTPSD = CTPSD->getInstantiatedFromMember()) {
-if (NewCTPSD->isMemberSpecialization())
+  while (!CTPSD->isMemberSpecialization()) {
+auto *NewCTPSD = CTPSD->getInstantiatedFromMember();
+if (!NewCTPSD)
   break;
 CTPSD = NewCTPSD;
   }
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1052,7 +1052,10 @@
   // If this is a member class of a specialization of a class template
   // and the corresponding decl has explicit visibility, use that.
   if (const auto *RD = dyn_cast(ND)) {
-CXXRecordDecl *InstantiatedFrom = RD->getInstantiatedFromMemberClass();
+const CXXRecordDecl *InstantiatedFrom =
+RD->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = RD->getInstantiatedFromMemberClass();
 if (InstantiatedFrom)
   return getVisibilityOf(InstantiatedFrom, kind);
   }
@@ -1086,16 +1089,13 @@
   }
   // Also handle function template specializations.
   if (const auto *fn = dyn_cast(ND)) {
-// If the function is a specialization of a template with an
-// explicit visibility attribute, use that.
-if (FunctionTemplateSpecializationInfo *templateInfo
-  = fn->getTemplateSpecializationInfo())
-  return getVisibilityOf(templateInfo->getTemplate()->getTemplatedDecl(),
-

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Alex Lorenz via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D13419#581356, @loladiro wrote:

> Hmm, the rebased version of this introduces new errors in 
> `test/Modules/cxx-templates.cpp`. That test didn't exist when I wrote this 
> code and I'm not familiar with templates. @rsmith could you take a look, 
> error is:
>
>   error: 'error' diagnostics seen but not expected:
> File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: 
> definition of 'nested_cls_t' must be imported from module 
> 'cxx_templates_common.unimported' before it is required
> File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: 
> definition of 'nested_cls_t' must be imported from module 
> 'cxx_templates_common.unimported' before it is required
>


FWIW, I also found this issue while working on my patch and decided to add 
`expected-error 1+{{definition of}}` verified check to that line ( 
https://reviews.llvm.org/differential/changeset/?ref=558280=ignore-most).
 It seems that the preceding lines have this check as well, so I assumed that 
this diagnostic was expected and my patch fixed a bug that prevented this 
diagnostic from showing up.


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Alex Lorenz via cfe-commits]

arphaman added a comment.

I just noticed that this patch fixes a template crash that I have a patch for 
at https://reviews.llvm.org/D25942. I will abandon my patch since this patch 
fixes the other issue. Would you mind adding the test case from my patch to 
this patch? It's available at 
https://reviews.llvm.org/differential/changeset/?ref=558279=ignore-most
 .


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added a comment.

Hmm, the rebased version of this introduces new errors in 
`test/Modules/cxx-templates.cpp`. That test didn't exist when I wrote this code 
and I'm not familiar with templates. @rsmith could you take a look, error is:

  error: 'error' diagnostics seen but not expected:
File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: 
definition of 'nested_cls_t' must be imported from module 
'cxx_templates_common.unimported' before it is required
File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: 
definition of 'nested_cls_t' must be imported from module 
'cxx_templates_common.unimported' before it is required


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 76094.
loladiro added a comment.

Rebased patch


Repository:
  rL LLVM

https://reviews.llvm.org/D13419

Files:
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  test/CodeGenCXX/visibility.cpp

Index: test/CodeGenCXX/visibility.cpp
===
--- test/CodeGenCXX/visibility.cpp
+++ test/CodeGenCXX/visibility.cpp
@@ -1317,3 +1317,59 @@
   // CHECK-LABEL: define void @_ZN6test693foo1fEv
   // CHECK-HIDDEN-LABEL: define hidden void @_ZN6test693foo1fEv
 }
+
+namespace test70 {
+template 
+class foo {
+public:
+  T x;
+  template 
+  HIDDEN S AddS(S);
+  template 
+  class HIDDEN barS {
+  public:
+static S AddS2(foo x, S);
+  };
+  template 
+  class HIDDEN barZ {
+  public:
+template 
+static S AddSZ(foo x, S, Z);
+  };
+};
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+template 
+template 
+HIDDEN S foo::AddS(S y) {
+  return ((S)x) + y;
+}
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+template 
+template 
+HIDDEN S foo::barS::AddS2(foo x, S y) {
+  return ((S)x.x) + y;
+}
+
+// CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+// CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+template 
+template 
+template 
+HIDDEN S foo::barZ::AddSZ(foo x, S y, Z z) {
+  return ((S)x.x) + y + ((S)z);
+}
+
+extern template struct foo;
+template struct foo;
+
+void f() {
+  auto var = foo{5};
+  auto bar = var.AddS((long long)3);
+  auto bar2 = decltype(var)::barS::AddS2(var, 3);
+  auto bar3 = decltype(var)::barZ::AddSZ(var, 3, (char)0);
+}
+}
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1346,17 +1346,19 @@
   if (auto *TD = dyn_cast(this)) {
 auto From = TD->getInstantiatedFrom();
 if (auto *CTD = From.dyn_cast()) {
-  while (auto *NewCTD = CTD->getInstantiatedFromMemberTemplate()) {
-if (NewCTD->isMemberSpecialization())
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }
   return CTD->getTemplatedDecl()->getDefinition();
 }
 if (auto *CTPSD =
 From.dyn_cast()) {
-  while (auto *NewCTPSD = CTPSD->getInstantiatedFromMember()) {
-if (NewCTPSD->isMemberSpecialization())
+  while (!CTPSD->isMemberSpecialization()) {
+auto *NewCTPSD = CTPSD->getInstantiatedFromMember();
+if (!NewCTPSD)
   break;
 CTPSD = NewCTPSD;
   }
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1052,7 +1052,10 @@
   // If this is a member class of a specialization of a class template
   // and the corresponding decl has explicit visibility, use that.
   if (const auto *RD = dyn_cast(ND)) {
-CXXRecordDecl *InstantiatedFrom = RD->getInstantiatedFromMemberClass();
+const CXXRecordDecl *InstantiatedFrom =
+RD->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = RD->getInstantiatedFromMemberClass();
 if (InstantiatedFrom)
   return getVisibilityOf(InstantiatedFrom, kind);
   }
@@ -1086,16 +1089,13 @@
   }
   // Also handle function template specializations.
   if (const auto *fn = dyn_cast(ND)) {
-// If the function is a specialization of a template with an
-// explicit visibility attribute, use that.
-if (FunctionTemplateSpecializationInfo *templateInfo
-  = fn->getTemplateSpecializationInfo())
-  return getVisibilityOf(templateInfo->getTemplate()->getTemplatedDecl(),
- kind);
+// If the function is a template specialization or a member of
+// a specialized class template and the corresponding decl has
+// explicit visibility, use that.
+FunctionDecl *InstantiatedFrom = fn->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = fn->getInstantiatedFromMemberFunction();
 
-// If the function is a member of a specialization of a class template
-// and the corresponding decl has explicit visibility, use that.
-FunctionDecl *InstantiatedFrom = fn->getInstantiatedFromMemberFunction();
 if (InstantiatedFrom)
   return getVisibilityOf(InstantiatedFrom, kind);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added a comment.

Since this was approved, I'll rebase and commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

Looks like patch was not committed.


Repository:
  rL LLVM

https://reviews.llvm.org/D13419



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

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Reid Kleckner via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rL LLVM

http://reviews.llvm.org/D13419



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

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added a comment.

bump again


Repository:
  rL LLVM

http://reviews.llvm.org/D13419



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

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro added a comment.

Bumping this again.


Repository:
  rL LLVM

http://reviews.llvm.org/D13419



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

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro updated this revision to Diff 36565.
loladiro added a comment.

Address review comment re loop structure. Reword comment that had a typo to 
both fix the typo and make the intent of the code more clear.


Repository:
  rL LLVM

http://reviews.llvm.org/D13419

Files:
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  test/CodeGenCXX/visibility.cpp

Index: test/CodeGenCXX/visibility.cpp
===
--- test/CodeGenCXX/visibility.cpp
+++ test/CodeGenCXX/visibility.cpp
@@ -1314,3 +1314,57 @@
   // CHECK-LABEL: define void @_ZN6test693foo1fEv
   // CHECK-HIDDEN-LABEL: define hidden void @_ZN6test693foo1fEv
 }
+
+namespace test70 {
+  template < typename T > class foo {
+  public:
+  T x;
+  template < typename S >
+  HIDDEN S AddS(S);
+  template < typename S > class HIDDEN barS {
+public:
+  static S AddS2(foo x, S);
+  };
+  template < typename S > class HIDDEN barZ {
+  public:
+template < typename Z >
+  static S AddSZ(foo x, S, Z);
+  };
+  };
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+  template < typename T >
+  template < typename S >
+  HIDDEN S foo::AddS(S y) {
+  return ((S) x) + y;
+  }
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+  template < typename T >
+  template < typename S >
+  HIDDEN S foo::barS::AddS2(foo x, S y) {
+  return ((S) x.x) + y;
+  }
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+  template < typename T >
+  template < typename S >
+  template < typename Z >
+  HIDDEN S foo::barZ::AddSZ(foo x, S y, Z z) {
+  return ((S) x.x) + y + ((S) z);
+  }
+
+  extern template struct foo;
+  template struct foo;
+
+  void f() {
+  auto var = foo{5};
+  auto bar = var.AddS((long long)3);
+  auto bar2 = decltype(var)::barS::AddS2(var,3);
+  auto bar3 = decltype(var)::barZ::AddSZ(var,3,(char)0);
+  }
+}
+
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1261,17 +1261,19 @@
   if (auto *TD = dyn_cast(this)) {
 auto From = TD->getInstantiatedFrom();
 if (auto *CTD = From.dyn_cast()) {
-  while (auto *NewCTD = CTD->getInstantiatedFromMemberTemplate()) {
-if (NewCTD->isMemberSpecialization())
+  while (!CTD->isMemberSpecialization()) {
+auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
+if (!NewCTD)
   break;
 CTD = NewCTD;
   }
   return CTD->getTemplatedDecl()->getDefinition();
 }
 if (auto *CTPSD =
 From.dyn_cast()) {
-  while (auto *NewCTPSD = CTPSD->getInstantiatedFromMember()) {
-if (NewCTPSD->isMemberSpecialization())
+  while (!CTPSD->isMemberSpecialization()) {
+auto *NewCTPSD = CTPSD->getInstantiatedFromMember();
+if (!NewCTPSD)
   break;
 CTPSD = NewCTPSD;
   }
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1049,7 +1049,9 @@
   // If this is a member class of a specialization of a class template
   // and the corresponding decl has explicit visibility, use that.
   if (const CXXRecordDecl *RD = dyn_cast(ND)) {
-CXXRecordDecl *InstantiatedFrom = RD->getInstantiatedFromMemberClass();
+const CXXRecordDecl *InstantiatedFrom = RD->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = RD->getInstantiatedFromMemberClass();
 if (InstantiatedFrom)
   return getVisibilityOf(InstantiatedFrom, kind);
   }
@@ -1084,16 +1086,12 @@
   }
   // Also handle function template specializations.
   if (const FunctionDecl *fn = dyn_cast(ND)) {
-// If the function is a specialization of a template with an
-// explicit visibility attribute, use that.
-if (FunctionTemplateSpecializationInfo *templateInfo
-  = fn->getTemplateSpecializationInfo())
-  return getVisibilityOf(templateInfo->getTemplate()->getTemplatedDecl(),
- kind);
-
-// If the function is a member of a specialization of a class template
-// and the corresponding decl has explicit visibility, use that.
-FunctionDecl *InstantiatedFrom = fn->getInstantiatedFromMemberFunction();
+// If the function is a template specialization or a member of
+// a specialized class template and the corresponding decl has
+// explicit visibility, use that.
+FunctionDecl *InstantiatedFrom = fn->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = fn->getInstantiatedFromMemberFunction();

Re: [PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/AST/Decl.cpp:1089
@@ -1086,10 +1088,3 @@
   if (const FunctionDecl *fn = dyn_cast(ND)) {
-// If the function is a specialization of a template with an
-// explicit visibility attribute, use that.
-if (FunctionTemplateSpecializationInfo *templateInfo
-  = fn->getTemplateSpecializationInfo())
-  return getVisibilityOf(templateInfo->getTemplate()->getTemplatedDecl(),
- kind);
-
-// If the function is a member of a specialization of a class template
+// If the function is a member of a specialization of a some template
 // and the corresponding decl has explicit visibility, use that.

typo "of a some"


Comment at: lib/AST/DeclCXX.cpp:1264-1269
@@ -1263,7 +1263,8 @@
 if (auto *CTD = From.dyn_cast()) {
-  while (auto *NewCTD = CTD->getInstantiatedFromMemberTemplate()) {
+  auto *NewCTD = CTD;
+  do {
+CTD = NewCTD;
 if (NewCTD->isMemberSpecialization())
   break;
-CTD = NewCTD;
-  }
+  } while ((NewCTD = CTD->getInstantiatedFromMemberTemplate()));
   return CTD->getTemplatedDecl()->getDefinition();

This loop structure is not very clear. How about:

  while (!CTD->isMemberSpecialization()) {
auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
if (!NewCTD)
  break;
CTD = NewCTD;
  }

... or

  while (auto *FromCTD = CTD->isMemberSpecialization()
 ? nullptr
 : CTD->getInstantiatedFromMemberTemplate())
CTD = FromCTD;


Repository:
  rL LLVM

http://reviews.llvm.org/D13419



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

[PATCH] D13419: Fix several problems at the intersection of template instantiations and visibility

[Keno Fischer via cfe-commits]

loladiro created this revision.
loladiro added reviewers: aaron.ballman, rsmith, rnk.
loladiro added a subscriber: cfe-commits.
loladiro set the repository for this revision to rL LLVM.

When we were looking at a template instantiation, that itself was a template 
instantiation
(say a templated member of a templated class), we weren't looking back far 
enough along
the chain of instantiations to find a VisibilityAttr (which we don't copy when 
instantiating
templates). This patch attempts to address that as well as adding a few test 
cases for these
situations.

Repository:
  rL LLVM

http://reviews.llvm.org/D13419

Files:
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  test/CodeGenCXX/visibility.cpp

Index: test/CodeGenCXX/visibility.cpp
===
--- test/CodeGenCXX/visibility.cpp
+++ test/CodeGenCXX/visibility.cpp
@@ -1314,3 +1314,57 @@
   // CHECK-LABEL: define void @_ZN6test693foo1fEv
   // CHECK-HIDDEN-LABEL: define hidden void @_ZN6test693foo1fEv
 }
+
+namespace test70 {
+  template < typename T > class foo {
+  public:
+  T x;
+  template < typename S >
+  HIDDEN S AddS(S);
+  template < typename S > class HIDDEN barS {
+public:
+  static S AddS2(foo x, S);
+  };
+  template < typename S > class HIDDEN barZ {
+  public:
+template < typename Z >
+  static S AddSZ(foo x, S, Z);
+  };
+  };
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4AddSIxEET_S3_
+  template < typename T >
+  template < typename S >
+  HIDDEN S foo::AddS(S y) {
+  return ((S) x) + y;
+  }
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barSIxE5AddS2ES1_x
+  template < typename T >
+  template < typename S >
+  HIDDEN S foo::barS::AddS2(foo x, S y) {
+  return ((S) x.x) + y;
+  }
+
+  // CHECK: define linkonce_odr hidden i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+  // CHECK-NOT: define linkonce_odr i64 @_ZN6test703fooIiE4barZIxE5AddSZIcEExS1_xT_
+  template < typename T >
+  template < typename S >
+  template < typename Z >
+  HIDDEN S foo::barZ::AddSZ(foo x, S y, Z z) {
+  return ((S) x.x) + y + ((S) z);
+  }
+
+  extern template struct foo;
+  template struct foo;
+
+  void f() {
+  auto var = foo{5};
+  auto bar = var.AddS((long long)3);
+  auto bar2 = decltype(var)::barS::AddS2(var,3);
+  auto bar3 = decltype(var)::barZ::AddSZ(var,3,(char)0);
+  }
+}
+
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1261,20 +1261,22 @@
   if (auto *TD = dyn_cast(this)) {
 auto From = TD->getInstantiatedFrom();
 if (auto *CTD = From.dyn_cast()) {
-  while (auto *NewCTD = CTD->getInstantiatedFromMemberTemplate()) {
+  auto *NewCTD = CTD;
+  do {
+CTD = NewCTD;
 if (NewCTD->isMemberSpecialization())
   break;
-CTD = NewCTD;
-  }
+  } while ((NewCTD = CTD->getInstantiatedFromMemberTemplate()));
   return CTD->getTemplatedDecl()->getDefinition();
 }
 if (auto *CTPSD =
 From.dyn_cast()) {
-  while (auto *NewCTPSD = CTPSD->getInstantiatedFromMember()) {
+  auto *NewCTPSD = CTPSD;
+  do {
+CTPSD = NewCTPSD;
 if (NewCTPSD->isMemberSpecialization())
   break;
-CTPSD = NewCTPSD;
-  }
+  } while ((NewCTPSD = CTPSD->getInstantiatedFromMember()));
   return CTPSD->getDefinition();
 }
   }
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1049,7 +1049,9 @@
   // If this is a member class of a specialization of a class template
   // and the corresponding decl has explicit visibility, use that.
   if (const CXXRecordDecl *RD = dyn_cast(ND)) {
-CXXRecordDecl *InstantiatedFrom = RD->getInstantiatedFromMemberClass();
+const CXXRecordDecl *InstantiatedFrom = RD->getTemplateInstantiationPattern();
+if (!InstantiatedFrom)
+  InstantiatedFrom = RD->getInstantiatedFromMemberClass();
 if (InstantiatedFrom)
   return getVisibilityOf(InstantiatedFrom, kind);
   }
@@ -1084,16 +1086,11 @@
   }
   // Also handle function template specializations.
   if (const FunctionDecl *fn = dyn_cast(ND)) {
-// If the function is a specialization of a template with an
-// explicit visibility attribute, use that.
-if (FunctionTemplateSpecializationInfo *templateInfo
-  = fn->getTemplateSpecializationInfo())
-  return getVisibilityOf(templateInfo->getTemplate()->getTemplatedDecl(),
- kind);
-
-// If the function is a member of a specialization of a class template
+// If the function is a member of a specialization of a some template
 // and the