Fix several problems at the intersection of template instantiations and visibility

[loladiro]

Summary

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.

[rsmith]

lib/AST/Decl.cpp

1088–1089

typo "of a some"

lib/AST/DeclCXX.cpp

1263–1269

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;

[loladiro]

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.

[loladiro]

Bumping this again.

[loladiro]

bump again

[rnk]

lgtm

[Eugene.Zelenko]

Looks like patch was not committed.

[loladiro]

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

[loladiro]

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

[loladiro]

Rebased patch

[arphaman]

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&whitespace=ignore-most .

[arphaman]

In 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&whitespace=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.

[loladiro]

Add the expected-error annotation

[rsmith]

Please factor out the fix for getTemplateInstantiationPattern and commit it separately. D25942 has a testcase for that portion of this change.

lib/AST/Decl.cpp

1054–1055

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<typename T> struct A {
  template<typename U> struct __attribute__((visibility("hidden"))) B;
};
template<> template<typename U> struct A<int>::B { virtual void f() {} };
void g() { A<int>::B<int>().f(); }

Here, A<int>::B<int> gets hidden visibility, even though it was instantiated from the member template specialization A<int>::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.

1092–1093

Likewise.

lib/AST/DeclCXX.cpp

1264–1269

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

[loladiro]

lib/AST/DeclCXX.cpp

1264–1269

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.

[rsmith]

lib/AST/DeclCXX.cpp

1264–1269

Since this is only currently used by the modules visibility code, you'd presumably need something like this:

// submodule a.x
template<typename T> struct A {
  template<typename U> static const int n;
};

// submodule a.y
import a.x
template<typename T> template<typename U> const int A<T>::n = 1;

// submodule a.z
import a.x
template<> template<typename U> const int A<int>::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<int>::n<int> == 2);

[loladiro]

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?

lib/AST/Decl.cpp

getExplicitVisibilityAux(const NamedDecl *ND,
 
   // Check the declaration itself first.
   if (Optional<Visibility> V = getVisibilityOf(ND, kind))
     return V;
 
   // 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<CXXRecordDecl>(ND)) {
-    CXXRecordDecl *InstantiatedFrom = RD->getInstantiatedFromMemberClass();
+    const CXXRecordDecl *InstantiatedFrom = RD->getTemplateInstantiationPattern();
+    if (!InstantiatedFrom)
+      InstantiatedFrom = RD->getInstantiatedFromMemberClass();
     if (InstantiatedFrom)
       return getVisibilityOf(InstantiatedFrom, kind);
   }
 
   // If there wasn't explicit visibility there, and this is a
   // specialization of a class template, check for visibility
   // on the pattern.
   if (const ClassTemplateSpecializationDecl *spec
         = dyn_cast<ClassTemplateSpecializationDecl>(ND))
if (const VarDecl *Var = dyn_cast<VarDecl>(ND)) {
 
     if (const auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(Var))
       return getVisibilityOf(VTSD->getSpecializedTemplate()->getTemplatedDecl(),
                              kind);
 
     return None;
   }
   // Also handle function template specializations.
   if (const FunctionDecl *fn = dyn_cast<FunctionDecl>(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();
     if (InstantiatedFrom)
       return getVisibilityOf(InstantiatedFrom, kind);
 
     return None;
   }
 
   // The visibility of a template is stored in the templated decl.
   if (const TemplateDecl *TD = dyn_cast<TemplateDecl>(ND))

lib/AST/DeclCXX.cpp

CXXRecordDecl::setTemplateSpecializationKind(TemplateSpecializationKind TSK) {
   llvm_unreachable("Not a class template or member class specialization");
 }
 
 const CXXRecordDecl *CXXRecordDecl::getTemplateInstantiationPattern() const {
   // If it's a class template specialization, find the template or partial
   // specialization from which it was instantiated.
   if (auto *TD = dyn_cast<ClassTemplateSpecializationDecl>(this)) {
     auto From = TD->getInstantiatedFrom();
     if (auto *CTD = From.dyn_cast<ClassTemplateDecl *>()) {
-      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<ClassTemplatePartialSpecializationDecl *>()) {
-      while (auto *NewCTPSD = CTPSD->getInstantiatedFromMember()) {
-        if (NewCTPSD->isMemberSpecialization())
+      while (!CTPSD->isMemberSpecialization()) {
+        auto *NewCTPSD = CTPSD->getInstantiatedFromMember();
+        if (!NewCTPSD)
           break;
         CTPSD = NewCTPSD;
       }
       return CTPSD->getDefinition();
     }
   }
 
   if (MemberSpecializationInfo *MSInfo = getMemberSpecializationInfo()) {

test/CodeGenCXX/visibility.cpp

namespace test69 {
   namespace foo {
     void f() {};
   }
   namespace foo __attribute__((visibility("hidden"))) {
   }
   // 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<T> x, S);
+      };
+      template < typename S > class HIDDEN barZ {
+          public:
+    template < typename Z >
+          static S AddSZ(foo<T> 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<T>::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<T>::barS<S>::AddS2(foo<T> 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<T>::barZ<S>::AddSZ(foo<T> x, S y, Z z) {
+      return ((S) x.x) + y + ((S) z);
+  }
+
+  extern template struct foo<int>;
+  template struct foo<int>;
+
+  void f() {
+      auto var = foo<int>{5};
+      auto bar = var.AddS((long long)3);
+      auto bar2 = decltype(var)::barS<long long>::AddS2(var,3);
+      auto bar3 = decltype(var)::barZ<long long>::AddSZ(var,3,(char)0);
+  }
+}
+