r175326 - Rework the visibility computation algorithm in preparation

[Nico Weber]

This does indeed break the chromium build. Thanks for the heads-up, Rafael.

There are two problems, but they affect a small number of files.

1.) The problem you discussed above. Fixable on our end.

#include headers for Rect, Point, Size, Vector2d;
class Insets;  // Only forward declared
class UI_EXPORT Rect
    : public RectBase<Rect, Point, Size, Insets, Vector2d, int> {
  // Out-of-line methods of the superclass are now no longer exported,
  // since the superclass when it's inherited from, and class Insets isn't
  // marked as UI_EXPORT.
}

This is easily fixable by just adding UI_EXPORTS to the forward
declaration of Insets. From what I can tell, this is needed only in
two lines for all of chromium, so this isn't an issue.

(Saying "Chrome is relying on a quirk of GCC's translation model here"
sounds a bit too dismissive though: As far as I know, all this
visibility stuff is implementation-defined, and this code used to work
with gcc, msvc, and eg.g clang 3.2.

If you do decide to deviate from gcc here, it'd be nice if there was
some documentation. To the reader, it probably looks weird that two
out of thousands of class forward declarations require explicit
visibility annotations, so it'd be nice if I could include a link to
something that explains why this is needed for types used in template
type lists but can be omitted elsewhere.

But this is just a parenthetical remark. For chromium, this change
isn't an issue as I said above.)


2.) Visibility of specializations. Might need clang changes.

We have a few (two) places where we implement a specialization of a
template class method for just a few types. Example:

dhcp-172-28-82-245:src thakis$ cat test.cc
// This is typically in a header:
#include <string>

#define EXPORT __attribute__((visibility("default")))

template<typename T>
class MyTemplate {
public:
  int f() { return external_f(); }
  int EXPORT external_f();
};

template<>
EXPORT int MyTemplate<int>::external_f();

template<>
EXPORT int MyTemplate<std::string>::external_f();
// This in a client:
int main() {
  MyTemplate<int> i;
  MyTemplate<std::string> s;
  return i.f() + s.f();
}
// This in some library cc file:
template<>
int MyTemplate<int>::external_f() {
  return 41;
}

template<>
int MyTemplate<std::string>::external_f() {
  return 42;
}

external_f used to be exported:

dhcp-172-28-82-245:src thakis$ clang -c test.cc -fvisibility=hidden &&
nm -m test.o | grep external_f
0000000000000050 (__TEXT,__text) external __ZN10MyTemplateISsE10external_fEv
00000000000001f8 (__TEXT,__eh_frame) external
__ZN10MyTemplateISsE10external_fEv.eh
0000000000000040 (__TEXT,__text) external __ZN10MyTemplateIiE10external_fEv
00000000000001d0 (__TEXT,__eh_frame) external
__ZN10MyTemplateIiE10external_fEv.eh

Now, the int version is still exported but the string version isn't any longer:
dhcp-172-28-82-245:src thakis$ ~/src/llvm/Release+Asserts/bin/clang -c
test.cc -fvisibility=hidden && nm -m test.o | grep external_f
0000000000000050 (__TEXT,__text) private external
__ZN10MyTemplateISsE10external_fEv
00000000000001f8 (__TEXT,__eh_frame) private external
__ZN10MyTemplateISsE10external_fEv.eh
0000000000000040 (__TEXT,__text) external __ZN10MyTemplateIiE10external_fEv
00000000000001d0 (__TEXT,__eh_frame) external
__ZN10MyTemplateIiE10external_fEv.eh


The specialization is explicitly marked as visible, and since string
is in a system header I can't change its visibility. Is this also an
intended change?


Nico

On Wed, Feb 20, 2013 at 2:57 AM, John McCall <rjmccall at apple.com> wrote:
> On Feb 19, 2013, at 1:07 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>> Rework the visibility computation algorithm in preparation
>>> for distinguishing type vs. value visibility.
>>>
>>> The changes to the visibility of explicit specializations
>>> are intentional.
>>
>> Thanks. It is indeed much cleaner to ignore other sources when a
>> specialization explicitly says what visibility should be used. A
>> change that looks unintended is that in
>>
>>  struct HIDDEN foo {
>>  };
>>  template <class P>
>>  struct bar {
>>    static DEFAULT void zed();
>>  };
>>  template <class P>
>>  void bar<P>::zed() {
>>  }
>>
>>  //template class bar<foo>;
>>
>>  void test() {
>>    bar<foo>::zed();
>>  }
>>
>> We produce a hidden symbol for zed, but uncommenting the explicit
>> template instantiation causes us to produce a default symbol. The idea
>> was for explicit template instantiations to make a difference only if
>> they have an attribute, no?
>
> Yeah, we should not have been ignoring template argument visibility
> in explicit instantiations just because there was a (pattern) member
> with explicit visibility.  This is fixed in r175587.
>
> John.
>