[clang] Handle instantiated members to determine visibility

[asavonic]

As reported in issue #103477, visibility of instantiated member functions used to be ignored when calculating visibility of a specialization.

This patch modifies getLVForClassMember to look up for a source template for an instantiated member, and changes mergeTemplateLV to apply it.

A similar issue was reported in #31462, but it seems that extern declaration with visibility prevents the function from being emitted as hidden. This behavior seems correct, even though GCC emits it as with default visibility instead.

Both tests from #103477 and #31462 are added as LIT tests test72 and test73 respectively.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Andrew Savonichev (asavonic)

Changes

As reported in issue #103477, visibility of instantiated member functions used to be ignored when calculating visibility of a specialization.

This patch modifies getLVForClassMember to look up for a source template for an instantiated member, and changes mergeTemplateLV to apply it.

A similar issue was reported in #31462, but it seems that extern declaration with visibility prevents the function from being emitted as hidden. This behavior seems correct, even though GCC emits it as with default visibility instead.

Both tests from #103477 and #31462 are added as LIT tests test72 and test73 respectively.

---

Full diff: https://github.com/llvm/llvm-project/pull/136128.diff

2 Files Affected:

• (modified) clang/lib/AST/Decl.cpp (+10-3)
• (modified) clang/test/CodeGenCXX/visibility.cpp (+37-1)

diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index ad1cb01592e9b..b59619892979a 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -400,9 +400,9 @@ void LinkageComputer::mergeTemplateLV(
   FunctionTemplateDecl *temp = specInfo->getTemplate();
   // Merge information from the template declaration.
   LinkageInfo tempLV = getLVForDecl(temp, computation);
-  // The linkage of the specialization should be consistent with the
-  // template declaration.
-  LV.setLinkage(tempLV.getLinkage());
+  // The linkage and visibility of the specialization should be
+  // consistent with the template declaration.
+  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
 
   // Merge information from the template parameters.
   LinkageInfo paramsLV =
@@ -1051,6 +1051,13 @@ LinkageComputer::getLVForClassMember(const NamedDecl *D,
     if (const auto *redeclTemp = dyn_cast<RedeclarableTemplateDecl>(temp)) {
       if (isExplicitMemberSpecialization(redeclTemp)) {
         explicitSpecSuppressor = temp->getTemplatedDecl();
+      } else if (const RedeclarableTemplateDecl *from =
+                     redeclTemp->getInstantiatedFromMemberTemplate()) {
+        // If no explicit visibility is specified yet, and this is an
+        // instantiated member of a template, look up visibility there
+        // as well.
+        LinkageInfo fromLV = from->getLinkageAndVisibility();
+        LV.mergeMaybeWithVisibility(fromLV, considerVisibility);
       }
     }
   }
diff --git a/clang/test/CodeGenCXX/visibility.cpp b/clang/test/CodeGenCXX/visibility.cpp
index e1061f3dbd18f..b69278a71d48e 100644
--- a/clang/test/CodeGenCXX/visibility.cpp
+++ b/clang/test/CodeGenCXX/visibility.cpp
@@ -1457,9 +1457,45 @@ namespace test71 {
   // CHECK-LABEL: declare hidden noundef i32 @_ZN6test713fooIiE3zedEv(
   // CHECK-LABEL: define linkonce_odr noundef i32 @_ZN6test713fooIiE3barIiEET_v(
   // CHECK-LABEL: define linkonce_odr hidden noundef i64 @_ZN6test713fooIlE3zedEv(
-  // CHECK-LABEL: define linkonce_odr noundef i32 @_ZN6test713fooIlE3barIiEET_v(
+  // CHECK-LABEL: define linkonce_odr hidden noundef i32 @_ZN6test713fooIlE3barIiEET_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 i64 @_ZN6test713fooIlE3zedEv(
   // CHECK-HIDDEN-LABEL: define linkonce_odr hidden noundef i32 @_ZN6test713fooIlE3barIiEET_v(
 }
+
+// https://github.com/llvm/llvm-project/issues/103477
+namespace test72 {
+  template <class a>
+  struct t {
+    template <int>
+    static HIDDEN void bar() {}
+  };
+
+  void test() {
+      t<char>::bar<1>();
+  }
+  // CHECK-LABEL: define linkonce_odr hidden void @_ZN6test721tIcE3barILi1EEEvv(
+  // CHECK-HIDDEN-LABEL: define linkonce_odr hidden void @_ZN6test721tIcE3barILi1EEEvv(
+}
+
+// https://github.com/llvm/llvm-project/issues/31462
+namespace test73 {
+  template <class T> struct s {
+    template <class U>
+    __attribute__((__visibility__("hidden"))) U should_not_be_exported();
+  };
+
+  template <class T> template <class U> U s<T>::should_not_be_exported() {
+    return U();
+  }
+
+  extern template struct __attribute__((__visibility__("default"))) s<int>;
+
+  int f() {
+    s<int> o;
+    return o.should_not_be_exported<int>();
+  }
+  // CHECK-LABEL: define linkonce_odr noundef i32 @_ZN6test731sIiE22should_not_be_exportedIiEET_v(
+  // CHECK-HIDDEN-LABEL: define linkonce_odr noundef i32 @_ZN6test731sIiE22should_not_be_exportedIiEET_v(
+}

[asavonic]

Libcxx tests failed. I'll check them tomorrow.

Failed Tests (15):
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/extents/assert.conversion.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/extents/assert.ctor_from_array.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/extents/assert.ctor_from_integral.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/extents/assert.ctor_from_span.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/extents/assert.obs.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_left/assert.conversion.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_left/assert.ctor.extents.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_right.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_stride.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_right/assert.conversion.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_right/assert.ctor.extents.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_right/assert.ctor.layout_left.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_right/assert.ctor.layout_stride.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_array.pass.cpp
  llvm-libc++-shared.cfg.in :: libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_span.pass.cpp

# | /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-bt7k7-1/llvm-project/github-pull-requests/libcxx/test/libcxx/containers/views/mdspan/extents/assert.conversion.pass.cpp:39:29: error: declaration of 'dynamic_extent' must be imported from module 'std.span.fwd' before it is required
# |    39 |   constexpr size_t D = std::dynamic_extent;
# |       |                             ^
# | /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-bt7k7-1/llvm-project/github-pull-requests/build/runtimes/runtimes-bins/libcxx/test-suite-install/include/c++/v1/__fwd/span.h:28:25: note: declaration here is not visible
# |    28 | inline constexpr size_t dynamic_extent = numeric_limits<size_t>::max();
# |       |                         ^
# | 1 error generated.

[Fznamznon]

Libcxx tests failed. I'll check them tomorrow.

IMO they fail now in all PRs, so this is likely unrelated.

[zyn0217]

FYI the libc++ CI has been fixed so all you need to do is to rebase your branch

[erichkeane]

This seems reasonable to me, but I'd like to see if @mizvekov has thoughts?

[mizvekov]

Thanks! LGTM as well.

[asavonic]

Thanks everyone for review!
Test issues were indeed fixed after rebase.

[zyn0217]

I was going to say we need a release note but you have merged it :)

[asavonic]

Sure, I'll post another patch to update release notes.

[jplehr]

I think this broke one of our bots: https://lab.llvm.org/buildbot/#/builders/10/builds/3726/steps/10/logs/stdio

The bot was red for another issue at the time, but the stack trace includes methods that are touched in this PR.
This is part of the stack trace:

#12 0x0000748dc31b00cb clang::LinkageComputer::computeTypeLinkageInfo(clang::Type const*) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0xbb00cb)
#13 0x0000748dc31b0b9b clang::LinkageComputer::getTypeLinkageAndVisibility(clang::Type const*) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0xbb0b9b)
#14 0x0000748dc2af0258 clang::LinkageComputer::getLVForTemplateArgumentList(llvm::ArrayRef<clang::TemplateArgument>, clang::LVComputationKind) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0x4f0258)
#15 0x0000748dc2af05d1 clang::LinkageComputer::mergeTemplateLV(clang::LinkageInfo&, clang::ClassTemplateSpecializationDecl const*, clang::LVComputationKind) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0x4f05d1)
#16 0x0000748dc2af120c clang::LinkageComputer::getLVForNamespaceScopeDecl(clang::NamedDecl const*, clang::LVComputationKind, bool) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0x4f120c)
#17 0x0000748dc2aeee05 clang::LinkageComputer::getLVForDecl(clang::NamedDecl const*, clang::LVComputationKind) (/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/bin/../lib/../lib/libclangAST.so.21.0git+0x4eee05)

I'd appreciate if someone more familiar with the matter can have a look at it. I'm happy to assist with info on build config etc.

[DKLoehr]

This is breaking builds when compiling chromium as well, bisected to this change. Example build: https://ci.chromium.org/ui/p/chromium/builders/ci/ToTLinux%20(dbg)/31974/overview. Compiler output can be seen by clicking [raw] under the compile step, but I'm working on a minimized reproduction now.

[erichkeane]

Hmm... do either of you (@jplehr @DKLoehr ) have the ability to provide the author a reduced example they can work with? We can revert in the meantime, but without a reduction provided (or AT LEAST a pre-processed example), there isn't anything we can really do about these reports.

Our crashes usually provide dumps (the location should be printed in the crash) which should provide the info needed to do so.

[jplehr]

I can try later today to get dumps/preprocessed source or maybe some better idea on how to reproduce the issue.
@jhuber6 if you have immediate thoughts on how to provide that (or maybe even what may be the issue), let me know.

[erichkeane]

Revert submitted here: #136317

When CI is ok with it, I'll merge it.

[DKLoehr]

I'm working on reducing my reproduction, but it's taking a while.

[DKLoehr]

Here's what cvise gave me; unfortunately, I'm in a rush so I don't have time to clean it up any further. It crashes when run with clang++ repro.cc -std=c++20 -Wno-everyt hing -fsyntax-only -ferror-limit=0

repro.cc:

template <bool, class _IfRes, class> using conditional_t = _IfRes;
template <class> void forward();
template <typename> class OnceCallback;
template <template <typename> class, typename> bool is_instantiation_v;
template <template <typename> class C, typename... Ts>
constexpr bool is_instantiation_v<C, C<Ts...>> = true;
template <template <typename> class C, typename T>
concept is_instantiation = is_instantiation_v<C, T>;
template <bool...> constexpr bool kIsWeakMethod = false;
template <typename> struct TypeList;
struct C;
template <int, typename> using DropTypeListItem = TypeList<C>;
template <typename, typename> struct MakeFunctionTypeImpl;
template <typename R, typename... Args>
struct MakeFunctionTypeImpl<R, TypeList<Args...>> {
  using Type = R(Args...);
};
template <typename R, typename ArgList>
using MakeFunctionType = MakeFunctionTypeImpl<R, ArgList>::Type;
template <typename Signature> using ExtractArgs = Signature;
template <typename Signature> using ExtractReturnType = Signature;
template <typename...> struct FunctorTraits;
template <typename> struct DecayedFunctorTraits;
template <typename R, typename Receiver>
struct DecayedFunctorTraits<R (Receiver::*)()> {
  using RunType = R;
  template <typename Method, typename ReceiverPtr>
  static void Invoke(Method, ReceiverPtr);
};
template <typename Functor>
struct FunctorTraits<Functor> : DecayedFunctorTraits<Functor> {};
template <bool, typename...> struct InvokeHelper;
template <typename Traits, typename ReturnType>
struct InvokeHelper<false, Traits, ReturnType> {
  template <typename Functor, typename BoundArgsTuple, typename... RunArgs>
  static void MakeItSo(Functor, BoundArgsTuple, RunArgs...) {
    Traits::Invoke(0, forward<RunArgs>...);
  }
};
template <bool is_once, typename T, typename StoredType = T,
          typename ForwardedType =
              conditional_t<is_once, StoredType, StoredType>>
using TransformToUnwrappedType = decltype(ForwardedType());
template <typename, typename> struct Invoker2;
template <typename Traits, typename R, typename... UnboundArgs>
struct Invoker2<Traits, R(UnboundArgs...)> {
  static void RunOnce() { RunImpl(0, 0); }
  template <typename Functor, typename BoundArgsTuple>
  static void RunImpl(Functor, BoundArgsTuple) {
    InvokeHelper<kIsWeakMethod<>, Traits, R>::MakeItSo(0, 0,
                                                       forward<UnboundArgs>...);
  }
};
template <template <typename> class CallbackT> struct BindHelper2 {
  static constexpr bool kIsOnce =
      is_instantiation<OnceCallback, CallbackT<void>>;
  template <typename Functor, typename... Args> static void BindImpl(Functor) {
    using Traits = FunctorTraits<TransformToUnwrappedType<kIsOnce, Functor>>;
    Invoker2<
        Traits,
        MakeFunctionType<
            ExtractReturnType<typename Traits::RunType>,
            DropTypeListItem<sizeof...(Args),
                             ExtractArgs<typename Traits::RunType>>>>::RunOnce;
  }
};
void (C::*BindOnce2void___trans_tmp_1)();
void BindOnce2void() {
  BindHelper2<OnceCallback>::BindImpl(BindOnce2void___trans_tmp_1);
}

[asavonic]

Thanks a lot @jplehr and @DKLoehr for helping with reproducers, and @erichkeane for taking care of the revert.

The compiler trips on LinkageComputer::computeTypeLinkageInfo for case Type::MemberPointer, where only CXXRecord is expected, it seems.
We get a FunctionProtoType instead, so getMostRecentCXXRecordDecl returns NULL:

MemberPointerType 0x55555571ea90 'type-parameter-0-0 (type-parameter-0-1::*)(void)' dependent
|-TemplateTypeParmType 0x5555556f73b0 'type-parameter-0-1' dependent depth 0 index 1
`-FunctionProtoType 0x55555571e9e0 'type-parameter-0-0 (void)' dependent cdecl
  `-TemplateTypeParmType 0x5555556f7630 'type-parameter-0-0' dependent depth 0 index 0

I'll figure out what to do with this case and send a new patch. The following helps to bypass the error, but perhaps we need to do something else for function types here.

diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 42e94d66d1a1..28a5fdc4c3a8 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4746,8 +4746,10 @@ LinkageInfo LinkageComputer::computeTypeLinkageInfo(const Type *T) {
     return computeTypeLinkageInfo(cast<ReferenceType>(T)->getPointeeType());
   case Type::MemberPointer: {
     const auto *MPT = cast<MemberPointerType>(T);
-    LinkageInfo LV =
-        getDeclLinkageAndVisibility(MPT->getMostRecentCXXRecordDecl());
+    LinkageInfo LV;
+    if (CXXRecordDecl *D = MPT->getMostRecentCXXRecordDecl()) {
+      LV.merge(getDeclLinkageAndVisibility(D));
+    }
     LV.merge(computeTypeLinkageInfo(MPT->getPointeeType()));
     return LV;
   }