[cfe-commits] [PR10113][patch] Don't consider template arguments when the visibility is already explicit

[Rafael Ávila de Espíndola]

 > This falls out of my question as well -- why doesn't merge DTRT here?
> Put another way, once we encounter an explicit visibility specification
> for a declaration, why would we ever merge it away? (This is distinct
> from computing a visibility for a declaration from that of a set of N
> other declarations, some of which use an explicit visibility spec, some
> of which do not.) I feel like we should have better logic to make an
> explicit visibility spec trump all else while still merging linkage....
> The current logic for merging of explicitly specified visibility makes
> no sense to me.

I implemented this in the patch I emailed earlier. While it is correct
and matches what gcc does, we do miss some cases that we correctly mark
as hidden and gcc doesn't. For example

-----------------------------------------
template<class T> class __attribute__((visibility("default"))) foo {
  void bar() { }
};
class __attribute__((visibility("hidden"))) zed;
template class foo<zed>;
----------------------------------------

Since the type foo<zed> cannot be used outside of this library, we can
make the symbol hidden.

In summary, what gcc and my previous patch do is ignore template
arguments if the template has an explicit visibility.

What clang currently does is always use the template arguments.

What I now think would be best is to use the template argument, only if
it has an explicit visibility. The attached implements does that.

The patch adds the new test from PR10113 and fixes the test

-----------------------------
struct __attribute__((visibility("hidden"))) A {};
template <class T> struct B {
  static void test5();
};
void test5() {
  B<A >::test5();
}
---------------------------

Which is a case I assume we were over compensating for not keeping track
of types having explicit visibility or not.

What the patch does is
* Track is a type has explicit visibility
* Drop the error prone LVPair and use LinkageInfo instead.

Cheers,
Rafael
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr10113.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120102/36df6bd3/attachment.ksh>