[pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default

[Allan McRae]

On 25/12/20 2:05 am, Emil Velikov wrote:
> On Thu, 24 Dec 2020 at 01:38, Eli Schwartz <eschwartz at archlinux.org> wrote:
>>
>> On 12/23/20 5:42 PM, Emil Velikov wrote:
>>> All the required public API is annotated with SYMEXPORT, so we can just
>>> add the meson notation, to hide all the symbols by default.
>>>
>>> Thus we no longer spill all the internal API into the global namespace.
>> Thanks for noticing this... it's a regression from autotools, which
>> contained in lib/libalpm/Makefile.am:
>>
>> if ENABLE_VISIBILITY_CC
>> if DARWIN
>> AM_CFLAGS += -fvisibility=hidden
>> else
>> AM_CFLAGS += -fvisibility=internal
>> endif
>> endif
>>
>>
>> I wonder if we had a good reason to use "internal" and if we should
>> continue to do so? IIUC it makes it slightly more optimized at the cost
>> of allowing pointers into private functions (e.g. callbacks) used by
>> other programs to segfault.
>>
> If the output of size&ls is any indication - there is little-to-no
> optimisation happening.
> The former produces the same numbers, while the latter claims that
> binaries built with "internal" are larger by 8 bytes (always).
> 
> Fwiw the above snippet is the first time I've seen anyone using
> "internal", after staring at various projects for 5+ years.
> Can bring it back, simply not sure it brings much.
> 
> Thanks
> Emil
> 
> $ ls
> 812336 build-internal/libalpm.so.12.0.1
> 812328 build-hidden/libalpm.so.12.0.1
> 337176 build-internal/pacman
> 337168 build-hidden/pacman
> 
> $ size
> 316708    3080     592  320380   4e37c build-internal/libalpm.so.12.0.1
> 316708    3080     592  320380   4e37c build-hidden/libalpm.so.12.0.1
> 155288    5040    5808  166136   288f8 build-internal/pacman
> 155288    5040    5808  166136   288f8 build-hidden/pacman
> 

It turns out, we have this:
#define SYMHIDDEN __attribute__((visibility("internal")))

But we never flag any functions with this.  That would enable a compiler
to optimize for speed and not compatibility, in which case using
-fvisibility=internal would make a difference.

I'm not sure if we removed any usage of that from the codebase, or if it
was never there...

Allan