[cfe-commits] [patch] Template instantiation and visibility

[Nico Weber]

On Sun, Apr 22, 2012 at 1:40 PM, Nico Weber <thakis at chromium.org> wrote:
> Hi Rafael,
>
> I tried building chrome with the components build with both trunk @
> r155304 and your patch.

Sorry, make that trunk @ r155317. 155304 is my llvm revision number 9_9

tests-MacBook-Pro-2:clang test$ ../../Release+Asserts/bin/clang --version
clang version 3.2 (trunk 155317) (llvm/trunk 155304)

> On trunk, all targets build fine. With this
> patch, target 'ipc' builds correctly (this was broken before your
> revert), so that's good. Target 'net' however fails to link:
>
> Undefined symbols for architecture i386:
>  "std::string* logging::MakeCheckOpString<std::string,
> std::string>(std::string const&, std::string const&, char const*)",
> referenced from:
>     net::HostResolverImpl::Job::AddRequest(scoped_ptr<net::HostResolverImpl::Request>)
> in net.host_resolver_impl.o
>     net::HostResolverImpl::Job::CancelRequest(net::HostResolverImpl::Request*)
> in net.host_resolver_impl.o
>     net::MimeUtil::MatchesMimeType(std::string const&, std::string
> const&) const in net.mime_util.o
>     net::CookieMonster::TrimDuplicateCookiesForKey(std::string
> const&, std::_Rb_tree_iterator<std::pair<std::string const,
> net::CookieMonster::CanonicalCookie*> >,
> std::_Rb_tree_iterator<std::pair<std::string const,
> net::CookieMonster::CanonicalCookie*> >) in net.cookie_monster.o
>     net::HttpAuthCache::Add(GURL const&, std::string const&,
> net::HttpAuth::Scheme, std::string const&, net::AuthCredentials
> const&, std::string const&) in net.http_auth_cache.o
> ld: symbol(s) not found for architecture i386
>
> Thanks for looking at this :-)
>
> Nico
>
>
> ps: I noticed that your patch doesn't add a new test case for whatever
> is different for target 'ipc'. If you want, I can try to build a
> reduced test case of what went wrong before your revert and check that
> in. I can do this for the problem mentioned in this mail too. (Will
> take until tomorrow though.)
>
> pps: Sorry, didn't Reply All the first time round.
>
> On Sun, Apr 22, 2012 at 11:49 AM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>> Now with the patch :-(
>>
>>
>> 2012/4/22 Rafael Espíndola <rafael.espindola at gmail.com>:
>>> Given
>>>
>>> namespace t1 {
>>>  template<typename T>
>>>  class DEFAULT foo {
>>>    void bar() {}
>>>  };
>>>  struct HIDDEN zed {
>>>  };
>>>  template class DEFAULT foo<zed>;
>>> }
>>> namespace t2 {
>>>  template<typename T>
>>>  class DEFAULT foo {
>>>    void bar() {}
>>>  };
>>>  struct HIDDEN zed {
>>>  };
>>>  template class foo<zed>;
>>> }
>>>
>>> gcc 4.7 produces a default symbol for the first test and a hidden one
>>> for the second one. This requires differentiating attributes in a
>>> template and attributes in an instantiation. Clang currently doesn't
>>> do that for any attributes, but Jeff tells me that "C++11 just says
>>> that attributes appertain to particular things, not how they propagate
>>> from templates to instantiations" so this looks like a valid c++
>>> extension by gcc.
>>>
>>> The attached patch:
>>>
>>> * Avoids cloning the visibility attribute during instantiations.
>>> * Avoids considering visibility info in a template when the
>>> instantiation has an attribute. This lets us get the first testcase
>>> right.
>>> * Goes back to selecting the minimum visibility in cases where we
>>> should look at the template visibility. This lets us get the second
>>> testcase right.
>>>
>>> The summary is that this fixes cases where where we were being
>>> unnecessary conservative in producing a default visibility but avoids
>>> breaking chrome's build.
>>>
>>> Cheers,
>>> Rafael