r175326 - Rework the visibility computation algorithm in preparation

John McCall rjmccall at apple.com
Thu Feb 28 10:00:58 PST 2013


On Feb 28, 2013, at 8:56 AM, Nico Weber <thakis at chromium.org> wrote:
> On Wed, Feb 27, 2013 at 8:05 PM, John McCall <rjmccall at apple.com> wrote:
>> On Feb 27, 2013, at 7:19 AM, Nico Weber <thakis at chromium.org> wrote:
>>> On Wed, Feb 27, 2013 at 3:32 PM, Nico Weber <thakis at chromium.org> wrote:
>>>> On Tue, Feb 26, 2013 at 11:37 PM, John McCall <rjmccall at apple.com> wrote:
>>>>> On Feb 26, 2013, at 2:36 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>>>>>> We should just not cache visibility for types.
>>>>>> 
>>>>>> Can do. I will send a patch.
>>>>> 
>>>>> Thanks!  As you're doing so, please change it to use the same LinkageInfo structure that NamedDecl uses.  You'll need to make it a non-member type and possibly pull it into its own header.
>>>> 
>>>> As of clang r176164 chromium's components build works again, with just
>>>> 6 additional explicit EXPORTs on predeclared types used in template
>>>> instantiations. Thanks for the help!
>>> 
>>> …while the clang build is happy with these changes, the gcc build
>>> breaks with them because gcc warns
>>> 
>>> test.cc:6: warning: type attributes ignored after type is already defined
>>> 
>>> on code like
>>> 
>>> $ cat test.cc
>>> #define EXPORT __attribute__((visibility("default")))
>>> 
>>> class Layer;
>>> class EXPORT Layer {
>>> };
>>> class EXPORT Layer;
>>> 
>>> Due to the template instantiation requiring the EXPORT on the
>>> declaration of Layer happening in one header and the definition of
>>> Layer happening in another, it depends on header include order if gcc
>>> is happy or not.
>>> 
>>> Do you have a better suggestion than putting the EXPORTed declaration
>>> of Layer into an #ifdef __clang__ block? Requiring this would be
>>> pretty ugly. (It's only 3 files for us so we'll live, but the user
>>> experience is a lot worse than in gcc / msvc.)
>> 
>> Maybe use an EXPORT_FORWARD_DECL macro?
>> 
>> That seems like a good bug to file against GCC, by the way.  There's
>> no reason for them to warn about a *redundant* attribute on a
>> redeclaration.
> 
> Since I know that you're all on the edge of your seats: As it turns
> out, everything works without any local chromium changes with clang
> r176164+, it just didn't occur to me to test that. (And if I revert
> r176164 locally, the problem reappears.)
> 
> The reason is that we have explicit instantiation declarations in our
> headers for the three classes that caused issues, and an explicit
> instantiation definition in one cc file. By the time the instantiation
> definition in the cc file is compiled, the real declaration of the
> template types has been included and its EXPORT annotation is picked
> up correctly since clang r176164.

Ah, I see.  And the other files might see the explicit instantiation as
having formally hidden visibility, but since the symbols are external and
the restriction didn't arise from an explicit hidden attribute, they don't
assume the symbols are defined within this linkage unit.  That's exactly
how the system's designed to work.

John.



More information about the cfe-commits mailing list