[Sema] Clone VisibilityAttr for functions in template instantiations

[MaskRay]

Summary

Fix https://github.com/llvm/llvm-project/issues/16561
Fix https://github.com/llvm/llvm-project/issues/31462

Modify the behavior from 7f90b7d4c29d560b57f6026f886adbf4d7ab4382 (2012) to
clone VisibilityAttr for functions. I think that commit fixed a behavior
but made us more difficult to align with GCC.

The new behavior aligns better with GCC. Specifically, an instantiated
FunctionTemplateDecl/CXXMethodDecl now inherits the original visibility
attribute (test51 and test71 in visibility.cpp).

The cloned VisibilityAttr is set to implicit. This is important for
template void HIDDEN zed<&y>(); to override the original VisibilityAttr (test51),
not getting an err_mismatched_visibility error.

For now, it's important not to clone VisibilityAttr for non-FunctionDecl as that
would break test38 (and change test35 in another way to be incompatible
with GCC).

[MaskRay]

remove a misleading comment

[MaskRay]

Ping:)

[efriedma]

Can you write the complete rule we're trying to follow here? Like, if you have a free function template, prioritize attributes in the following order ..., if you have a member function, use the following order ..., etc. I know that's a significant amount of writing, but I don't really have any intuition here about how it should work. So I'm not sure how to review this without a reference. (The closest thing I have to intuition is comparing the rules to the ones for C++ anonymous namespaces, but visibility is a lot more complicated.)

Given how complicated the rules are here, it might be easier to understand if you teach the relevant code in clang/lib/AST/Decl.cpp (getExplicitVisibilityAux, I think?) to explicitly check for attributes on the original template, instead of cloning the attribute.

[MaskRay]

In D153835#4478700, @efriedma wrote:

Can you write the complete rule we're trying to follow here? Like, if you have a free function template, prioritize attributes in the following order ..., if you have a member function, use the following order ..., etc. I know that's a significant amount of writing, but I don't really have any intuition here about how it should work. So I'm not sure how to review this without a reference. (The closest thing I have to intuition is comparing the rules to the ones for C++ anonymous namespaces, but visibility is a lot more complicated.)

The rules are quite complex and sorry that I am unable to describe them...

Given how complicated the rules are here, it might be easier to understand if you teach the relevant code in clang/lib/AST/Decl.cpp (getExplicitVisibilityAux, I think?) to explicitly check for attributes on the original template, instead of cloning the attribute.

If we don't clone VisibilityAttr, the best I can come up with is the following patch

--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -375,6 +375,9 @@ static bool shouldConsiderTemplateVisibility(const FunctionDecl *fn,
   if (!specInfo->isExplicitInstantiationOrSpecialization())
     return true;

+  if (specInfo->getTemplate()->getTemplatedDecl()->hasAttr<VisibilityAttr>())
+    return false;
+
   return !fn->hasAttr<VisibilityAttr>();
 }

test71 still fails (while this patch fixes test71). Here is my observation:

As confirmed by -fsyntax-only -Xclang -ast-dump, the explicit instantiation declaration (extern template struct DEFAULT foo<int>;) clones the FunctionTemplateDecl tree (including CXXMethodDecl) without cloning VisibilityAttr. This causes all hasAttr<VisibilityAttr>() code to behave incorrectly like the status quo.
By debugging TemplateDeclInstantiator, I believe the cloned FunctionTemplateDecl has no pointer referencing back to the original template.

I think this VisibilityAttr cloning patch matches GCC's spirit better and will enable simplification of the complex template instantiation/specialization rules.

[rjmccall]

We made a decision ten years or so ago to use a slightly different interpretation of the visibility attributes, so differences from GCC are not by themselves unexpected or problematic. In this case, I'm not sure I agree that it's a bug that an attribute on an explicit class template instantiation overrides attributes on member functions in the template pattern. Even if we decide to adopt a new rule here, though, I agree with Eli that this seems like a very heavy-handed way of implementing it; basically every line in this patch seems to be impactful in ways that are hard to anticipate.

[MaskRay]

In D153835#4481462, @rjmccall wrote:

We made a decision ten years or so ago to use a slightly different interpretation of the visibility attributes, so differences from GCC are not by themselves unexpected or problematic.

Sure, I think we should assess every corner case and decide whether it makes sense to match GCC compatibility.
I started the patch by noticing https://github.com/llvm/llvm-project/issues/31462 (D29157) to address a specific libc++ problem, not that I started to match a random GCC behavior.
I have noticed that libcxx/include/__config:592 says

// The inline should be removed once PR32114 is resolved
#      define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS inline _LIBCPP_HIDDEN

(The doc is old stale. This is something I want to improve next.)

In this case, I'm not sure I agree that it's a bug that an attribute on an explicit class template instantiation overrides attributes on member functions in the template pattern.

OK, I think this is where our opinions differ. I think that:

• (test51)(consensus bugfix?) the visibility attribute in template<foo *z> void DEFAULT zed() should affect template void zed<&x>();
• (test71)(differed opinion) the visibility attribute in template <class U> HIDDEN U bar(); should override the visibility attribute in extern template struct DEFAULT foo<int>;, like GCC does, as the method attribute is more specific.

Perhaps #libc folks and @erichkeane (who introduced MeaningfulToClassTemplateDefinition) can weigh in.

Even if we decide to adopt a new rule here, though, I agree with Eli that this seems like a very heavy-handed way of implementing it; basically every line in this patch seems to be impactful in ways that are hard to anticipate.

I actually think that the patch makes VisibilityAttr behave in a more normal way as the majority of attributes are cloned by instantiateTemplateAttribute in the tablegen-generated tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.
However, MeaningfulToClassTemplateDefinition attributes are rare and even few need to think about precedence for class attributes and method attributes.
This does make VisibilityAttr unusual, but I think the complexity resides mainly in the existing code, not the code I added in this patch.
I think that CodeGenCXX/visibility.cpp and a few related visibility tests are good enough to catch almost all cases we'd run into in practice.

[rjmccall]

I agree that the change to test51 is the right result, yes. The explicit visibility attribute on the template declaration should take precedence over the visibility of the types in the template parameter list, and then the explicit visibility attribute on the variables should take precedence over the visibility of the variable types, and then the instantiation should be a specialization of a default-visibility template with default-visibility arguments. But I think that logic ought to be fixed in getLVFor..., not by changing the basic propagation of attributes.

I actually think that the patch makes VisibilityAttr behave in a more normal way as the majority of attributes are cloned by instantiateTemplateAttribute in the tablegen-generated tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.

You're not really just cloning it, because making the cloned attribute implicit (i.e. giving it the effect of a visibility pragma instead of an attribute) is a huge difference.

[MaskRay]

In D153835#4481816, @rjmccall wrote:

I agree that the change to test51 is the right result, yes. The explicit visibility attribute on the template declaration should take precedence over the visibility of the types in the template parameter list, and then the explicit visibility attribute on the variables should take precedence over the visibility of the variable types, and then the instantiation should be a specialization of a default-visibility template with default-visibility arguments. But I think that logic ought to be fixed in getLVFor..., not by changing the basic propagation of attributes.

I actually think that the patch makes VisibilityAttr behave in a more normal way as the majority of attributes are cloned by instantiateTemplateAttribute in the tablegen-generated tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.

You're not really just cloning it, because making the cloned attribute implicit (i.e. giving it the effect of a visibility pragma instead of an attribute) is a huge difference.

OK, created D154774 as an alternative. I added more cases to test51 and test71. D154774 will change the tests in the same way that this VisibilityAttr cloning patch changes the tests.

I did not know that implicit VisibilityAttr is for #pragma GCC visibility push(...). I agree that reusing IsImplicit for an instantiated FunctionTemplateDecl/CXXMethodDecl has unclear interaction.

However, I am not fully convinced that avoiding cloning VisibilityAttr is the most clean way forward. That said, since D154774 will have the same effect as far as CodeGenCXX/visibility.cpp is concerned, I am fine not pursing this patch forward.
I wonder whether using another bit that is not IsImplicit will be better.

[MaskRay]

rebase
we may not go with this approach, but update to show the test difference

clang/include/clang/Basic/Attr.td

class Attr {
   // The things to which an attribute can appertain
   SubjectList Subjects;
   // The arguments allowed on an attribute
   list<Argument> Args = [];
   // Accessors which should be generated for the attribute.
   list<Accessor> Accessors = [];
   // Set to true for attributes with arguments which require delayed parsing.
   bit LateParsed = 0;
-  // Set to false to prevent an attribute from being propagated from a template
-  // to the instantiation.
-  bit Clone = 1;
   // Set to true for attributes which must be instantiated within templates
   bit TemplateDependent = 0;
   // Set to true for attributes that have a corresponding AST node.
   bit ASTNode = 1;
   // Set to true for attributes which have handler in Sema.
   bit SemaHandler = 1;
   // Set to true if this attribute doesn't need custom handling in Sema.
   bit SimpleHandler = 0;
def MatrixType : TypeAttr {
   let Subjects = SubjectList<[TypedefName], ErrorDiag>;
   let Args = [ExprArgument<"NumRows">, ExprArgument<"NumColumns">];
   let Documentation = [Undocumented];
   let ASTNode = 0;
   let PragmaAttributeSupport = 0;
 }
 
 def Visibility : InheritableAttr {
-  let Clone = 0;
   let Spellings = [GCC<"visibility">];
   let Args = [EnumArgument<"Visibility", "VisibilityType",
                            ["default", "hidden", "internal", "protected"],
                            ["Default", "Hidden", "Hidden", "Protected"]>];
   let MeaningfulToClassTemplateDefinition = 1;
   let Documentation = [Undocumented];
 }
 
 def TypeVisibility : InheritableAttr {
-  let Clone = 0;
   let Spellings = [Clang<"type_visibility">];
   let Args = [EnumArgument<"Visibility", "VisibilityType",
                            ["default", "hidden", "internal", "protected"],
                            ["Default", "Hidden", "Hidden", "Protected"]>];
 //  let Subjects = [Tag, ObjCInterface, Namespace];
   let Documentation = [Undocumented];
 }
 

clang/lib/Sema/SemaDeclAttr.cpp

 template <class T>
 static T *mergeVisibilityAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
                               typename T::VisibilityType value) {
   T *existingAttr = D->getAttr<T>();
   if (existingAttr) {
     typename T::VisibilityType existingValue = existingAttr->getVisibility();
     if (existingValue == value)
       return nullptr;
+    D->dropAttr<T>();
+    if (!existingAttr->isImplicit()) {
       S.Diag(existingAttr->getLocation(), diag::err_mismatched_visibility);
       S.Diag(CI.getLoc(), diag::note_previous_attribute);
-    D->dropAttr<T>();
+    }
   }
   return ::new (S.Context) T(S.Context, CI, value);
 }
 
 VisibilityAttr *Sema::mergeVisibilityAttr(Decl *D,
                                           const AttributeCommonInfo &CI,
                                           VisibilityAttr::VisibilityType Vis) {
   return ::mergeVisibilityAttr<VisibilityAttr>(*this, D, CI, Vis);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (auto *A = dyn_cast<OwnerAttr>(TmplAttr)) {
       continue;
     }
 
     if (auto *A = dyn_cast<SYCLKernelAttr>(TmplAttr)) {
       instantiateDependentSYCLKernelAttr(*this, TemplateArgs, *A, New);
       continue;
     }
 
+    if (auto *A = dyn_cast<VisibilityAttr>(TmplAttr)) {
+      if (isa<FunctionDecl>(Tmpl) && !New->hasAttr<VisibilityAttr>()) {
+        auto *NewA = A->clone(Context);
+        NewA->setImplicit(true);
+        New->addAttr(NewA);
+      }
+      continue;
+    }
+
+    if (auto *A = dyn_cast<TypeVisibilityAttr>(TmplAttr))
+      continue;
+
     assert(!TmplAttr->isPackExpansion());
     if (TmplAttr->isLateParsed() && LateAttrs) {
       // Late parsed attributes must be instantiated and attached after the
       // enclosing class has been instantiated.  See Sema::InstantiateClass.
       LocalInstantiationScope *Saved = nullptr;
       if (CurrentInstantiationScope)
         Saved = CurrentInstantiationScope->cloneScopes(OuterMostScope);
       LateAttrs->push_back(LateInstantiatedAttribute(TmplAttr, Saved, New));

clang/test/CodeGenCXX/visibility.cpp

struct DEFAULT bar {
     }
   };
   template void bar<&x>::zed();
   // CHECK-LABEL: define weak_odr hidden void @_ZN6test503barIXadL_ZNS_1xEEEE3zedEv
   // CHECK-HIDDEN-LABEL: define weak_odr hidden void @_ZN6test503barIXadL_ZNS_1xEEEE3zedEv
 }
 
 namespace test51 {
-  // Test that we use the visibility of struct foo when instantiating the
-  // template. Note that is a case where we disagree with gcc, it produces
-  // a default symbol.
-
   struct HIDDEN foo {
   };
   DEFAULT foo x, y;
   template<foo *z>
   void DEFAULT zed() {
   }
   template void zed<&x>();
-  // CHECK-LABEL: define weak_odr hidden void @_ZN6test513zedIXadL_ZNS_1xEEEEEvv
-  // CHECK-HIDDEN-LABEL: define weak_odr hidden void @_ZN6test513zedIXadL_ZNS_1xEEEEEvv
+  // CHECK-LABEL: define weak_odr void @_ZN6test513zedIXadL_ZNS_1xEEEEEvv
+  // CHECK-HIDDEN-LABEL: define weak_odr void @_ZN6test513zedIXadL_ZNS_1xEEEEEvv
 
   template void HIDDEN zed<&y>();
   // CHECK-LABEL: define weak_odr hidden void @_ZN6test513zedIXadL_ZNS_1yEEEEEvv(
   // CHECK-HIDDEN-LABEL: define weak_odr hidden void @_ZN6test513zedIXadL_ZNS_1yEEEEEvv(
 }
 
 namespace test52 {
   // Test that we use the linkage of struct foo when instantiating the
struct foo {
     static HIDDEN T zed();
     template <class U> HIDDEN U bar();
   };
   template <class T>
   T foo<T>::zed() { return {}; }
   template <class T> template <class U>
   U foo<T>::bar() { return {}; }
 
+  /// foo<int>::{zed,bar} instantiate the HIDDEN attributes.
   extern template struct DEFAULT foo<int>;
 
   int use() {
     foo<int> o;
     return o.zed() + o.bar<int>();
   }
-  /// FIXME: foo<int>::bar is hidden in GCC w/ or w/o -fvisibility=hidden.
   // CHECK-LABEL: declare hidden noundef i32 @_ZN6test713fooIiE3zedEv(
-  // CHECK-LABEL: define linkonce_odr noundef i32 @_ZN6test713fooIiE3barIiEET_v(
+  // CHECK-LABEL: define linkonce_odr hidden noundef i32 @_ZN6test713fooIiE3barIiEET_v(
   // CHECK-HIDDEN-LABEL: declare hidden noundef i32 @_ZN6test713fooIiE3zedEv(
-  // CHECK-HIDDEN-LABEL: define linkonce_odr noundef i32 @_ZN6test713fooIiE3barIiEET_v(
+  // CHECK-HIDDEN-LABEL: define linkonce_odr hidden noundef i32 @_ZN6test713fooIiE3barIiEET_v(
 }

clang/utils/TableGen/ClangAttrEmitter.cpp

void EmitClangAttrTemplateInstantiateHelper(const std::vector<Record *> &Attrs,
                                             bool AppliesToDecl) {
 
   OS << "  switch (At->getKind()) {\n";
   for (const auto *Attr : Attrs) {
     const Record &R = *Attr;
     if (!R.getValueAsBit("ASTNode"))
       continue;
     OS << "    case attr::" << R.getName() << ": {\n";
-    bool ShouldClone = R.getValueAsBit("Clone") &&
-                       (!AppliesToDecl ||
+    bool ShouldClone = (!AppliesToDecl ||
                         R.getValueAsBit("MeaningfulToClassTemplateDefinition"));
 
     if (!ShouldClone) {
       OS << "      return nullptr;\n";
       OS << "    }\n";
       continue;
     }