[cfe-commits] [patch][pr12001] Avoid increasing visibility in some cases

[Nico Weber]

Consider this program:

$ cat repro.ii
template<typename DataType>
class DeviceDataProviderImplBase { };

template<typename DataType>
class DeviceDataProvider {
  __attribute__((visibility("default")))
  static DeviceDataProviderImplBase<DataType>* DefaultFactoryFunction();
};

struct __attribute__((visibility("default"))) RadioData { };
typedef DeviceDataProviderImplBase<RadioData> RadioDataProviderImplBase;

typedef DeviceDataProvider<RadioData> RadioDataProvider;
template<>
RadioDataProviderImplBase* RadioDataProvider::DefaultFactoryFunction() { }



With gcc, and with clang before your patch, DefaultFactorFunction() is
a non-hidden symbol:
$ gcc -c repro.ii -w -fvisibility=hidden 2> /dev/null && objdump
--syms repro.o | grep
'_ZN18DeviceDataProviderI9RadioDataE22DefaultFactoryFunctionEv'
0000000000000000 g     F .text	0000000000000006
_ZN18DeviceDataProviderI9RadioDataE22DefaultFactoryFunctionEv


With clang with your patch, it becomes hidden:
$ chrome/src/third_party/llvm-build/Release+Asserts/bin/clang -c
repro.ii -w -fvisibility=hidden 2> /dev/null && objdump --syms repro.o
| grep '_ZN18DeviceDataProviderI9RadioDataE22DefaultFactoryFunctionEv'
0000000000000000 g     F .text	0000000000000006 .hidden
_ZN18DeviceDataProviderI9RadioDataE22DefaultFactoryFunctionEv


This breaks building some unit test binary in chromium land (the
browser itself builds fine either way).

Nico

On Wed, Feb 22, 2012 at 10:15 AM, Nico Weber <thakis at chromium.org> wrote:
> Doh, I think I found an issue with this patch :-/ I'm building a
> reduced test case now, I'll post it here in a few hours.
>
> Nico
>
> On Tue, Feb 21, 2012 at 5:57 PM, Eric Christopher <echristo at apple.com> wrote:
>>
>> On Feb 19, 2012, at 7:11 PM, Rafael Ávila de Espíndola <rafael.espindola at gmail.com> wrote:
>>
>>> Nico noticed that the patch included an infinite loop from when I
>>> renamed merge2 to mergeWithMin.
>>>
>>> An updated patch is attached.
>>
>> I think this is ok. Some more comments on the visibility that should be happening in the various testcases and where you use mergeWithMin would be helpful next time I need to review something through there.
>>
>> -eric