libc++: change default symbol visibility to hidden

libc++: change default symbol visibility to hidden

BUG=593874
R=sammc@chromium.org, thakis@chromium.org

Committed: 57c94085e893cd46aa1c5ad4ee9d7ee4670312a6

[Tom Anderson, 2017-05-19 20:35:23]

Description was changed from

==========
libc++: change default symbol visibility to hidden

BUG=593874
R=thakis@chromium.org
==========

to

==========
libc++: change default symbol visibility to hidden

BUG=593874
R=thakis@chromium.org,sammc@chromium.org
==========

[Tom Anderson, 2017-05-19 20:35:23]

thomasanderson@chromium.org changed reviewers:
+ sammc@chromium.org

[Tom Anderson, 2017-05-19 20:37:00]

thakis@ ptal

+sammc as author of
https://codereview.chromium.org/1145333004
subsequently, symbol_visibility_default was added in
https://codereview.chromium.org/1930753004

[Nico, 2017-05-19 20:43:53]

lgtm

pcc, do you remember anything about this (context:
https://bugs.chromium.org/p/chromium/issues/detail?id=607370)?

[pcc1, 2017-05-19 20:58:47]

This looks like the right change if we just want to change the visibility flags
passed to the compiler, but is this really all that is needed to change symbol
visibility in the object files? I was under the impression that this wouldn't be
enough because these libraries use macros to control visibility explicitly:

https://github.com/llvm-mirror/libcxx/blob/master/include/__config
https://github.com/llvm-mirror/libcxxabi/blob/master/include/__cxxabi_config.h

So I think we would also need to arrange to define
_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS and
_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS.

[Tom Anderson, 2017-05-19 21:12:52]

On 2017/05/19 20:58:47, pcc1 wrote:
> This looks like the right change if we just want to change the visibility
flags
> passed to the compiler, but is this really all that is needed to change symbol
> visibility in the object files? I was under the impression that this wouldn't
be
> enough because these libraries use macros to control visibility explicitly:
> 
> https://github.com/llvm-mirror/libcxx/blob/master/include/__config
> https://github.com/llvm-mirror/libcxxabi/blob/master/include/__cxxabi_config.h
> 
> So I think we would also need to arrange to define
> _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS and
> _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS.

Makes sense to me.  Currently the libc++ target is a shared_library so we need
the exported symbols, but eventually it will be made into a component so that it
will statically link on non-component builds.  When this change is made, we can
add those defines for non-component builds.  wdyt?

[Tom Anderson, 2017-05-20 02:05:14]

Description was changed from

==========
libc++: change default symbol visibility to hidden

BUG=593874
R=thakis@chromium.org,sammc@chromium.org
==========

to

==========
libc++: change default symbol visibility to hidden

BUG=593874
R=sammc@chromium.org, thakis@chromium.org

Committed: 57c94085e893cd46aa1c5ad4ee9d7ee4670312a6
==========

[Tom Anderson, 2017-05-20 02:05:16]

Committed patchset #2 (id:20001) manually as
57c94085e893cd46aa1c5ad4ee9d7ee4670312a6 (presubmit successful).

[pcc1, 2017-05-26 20:05:54]

On 2017/05/19 21:12:52, Tom Anderson wrote:
> On 2017/05/19 20:58:47, pcc1 wrote:
> > This looks like the right change if we just want to change the visibility
> flags
> > passed to the compiler, but is this really all that is needed to change
symbol
> > visibility in the object files? I was under the impression that this
wouldn't
> be
> > enough because these libraries use macros to control visibility explicitly:
> > 
> > https://github.com/llvm-mirror/libcxx/blob/master/include/__config
> >
https://github.com/llvm-mirror/libcxxabi/blob/master/include/__cxxabi_config.h
> > 
> > So I think we would also need to arrange to define
> > _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS and
> > _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS.
> 
> Makes sense to me.  Currently the libc++ target is a shared_library so we need
> the exported symbols, but eventually it will be made into a component so that
it
> will statically link on non-component builds.  When this change is made, we
can
> add those defines for non-component builds.  wdyt?

(looks like I was somehow dropped from the reviewer list)

Sounds good to me.