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

1098–1099

typo "of a some"

lib/AST/DeclCXX.cpp

1348–1354

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

1057–1058

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.

1096–1097

Likewise.

lib/AST/DeclCXX.cpp

1349–1354

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

[loladiro]

lib/AST/DeclCXX.cpp

1349–1354

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

1349–1354

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 auto *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 auto *spec = dyn_cast<ClassTemplateSpecializationDecl>(ND))
if (const auto *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 auto *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 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);
 
     return None;
   }
 
   // The visibility of a template is stored in the templated decl.
   if (const auto *TD = dyn_cast<TemplateDecl>(ND))
     return getVisibilityOf(TD->getTemplatedDecl(), kind);

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);
+}
+}

test/Modules/cxx-templates.cpp

void test() {
     cls<void>::nested_cls *nk1; // ok
     cls<void>::nested_cls_t<int> *nk2; // ok
     cls<void>::nested_cls_t<char> *nk3; // ok
     cls<int> uk1; // expected-error 1+{{explicit specialization of 'cls<int>' must be imported}} expected-error 1+{{definition of}}
     cls<int*> uk3; // expected-error 1+{{partial specialization of 'cls<type-parameter-0-0 *>' must be imported}} expected-error 1+{{definition of}}
     cls<char*> uk4; // expected-error 1+{{partial specialization of 'cls<type-parameter-0-0 *>' must be imported}} expected-error 1+{{definition of}}
     cls<void>::nested_cls unk1; // expected-error 1+{{explicit specialization of 'nested_cls' must be imported}} expected-error 1+{{definition of}}
     cls<void>::nested_cls_t<int> unk2; // expected-error 1+{{explicit specialization of 'nested_cls_t' must be imported}} expected-error 1+{{definition of}}
-    cls<void>::nested_cls_t<char> unk3; // expected-error 1+{{explicit specialization of 'nested_cls_t' must be imported}}
+    cls<void>::nested_cls_t<char> 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.
     cls<void>::nested_enum e; // ok
     (void)cls<void>::nested_enum::e; // expected-error 1+{{definition of 'nested_enum' must be imported}} expected-error 1+{{declaration of 'e'}}
 
     // For variable template specializations, no uses are allowed because
     // specializations can change the type.