[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a subscriber: erichkeane.
MaskRay added a comment.

In D153835#4481462 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 whether 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 <https://reviews.llvm.org/tag/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153835/new/

https://reviews.llvm.org/D153835