Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 12/28/20 10:15 PM, Allan McRae wrote:
On 25/12/20 2:05 am, Emil Velikov wrote:
On Thu, 24 Dec 2020 at 01:38, Eli Schwartz 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 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
3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1
3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1
15528850405808 166136 288f8 build-internal/pacman
15528850405808 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...
Hmm... only ever used on alpm_add_target (then _alpm_add_loadtarget) and
removed in commit 7f7da2b5fc01f46d28236384540c7ecfdac16a63 which added
the AM_CFLAGS instead and marked a bunch of symbols as SYMEXPORT.
But that define used to be for visibility("hidden"), then in commit
920b0d2049deb148efe89bfebda03d172b68c1f5 both the (unused) define and
the AM_CFLAGS got switched to internal: "this allows for slightly better
optimization".
Which is about as vague as our knowledge today. "Supposedly better and
probably works fine, but no one put out numbers on it".
idk, "hidden" is probably just fine.
--
Eli Schwartz
Bug Wrangler and Trusted User
OpenPGP_signature
Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 25/12/20 2:05 am, Emil Velikov wrote:
> On Thu, 24 Dec 2020 at 01:38, Eli Schwartz 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 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
> 3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1
> 3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1
> 15528850405808 166136 288f8 build-internal/pacman
> 15528850405808 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
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On Thu, 24 Dec 2020 at 01:38, Eli Schwartz 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 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 3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1 3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1 15528850405808 166136 288f8 build-internal/pacman 15528850405808 166136 288f8 build-hidden/pacman
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
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. -- Eli Schwartz Bug Wrangler and Trusted User OpenPGP_signature Description: OpenPGP digital signature
[pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
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. Signed-off-by: Emil Velikov --- meson.build | 2 ++ 1 file changed, 2 insertions(+) diff --git a/meson.build b/meson.build index 380564bb..6173f2b7 100644 --- a/meson.build +++ b/meson.build @@ -303,6 +303,7 @@ libcommon = static_library( 'common', libcommon_sources, include_directories : includes, + gnu_symbol_visibility : 'hidden', install : false) alpm_deps = [crypto_provider, libarchive, libcurl, libintl, gpgme] @@ -312,6 +313,7 @@ libalpm_a = static_library( libalpm_sources, link_whole: libcommon, include_directories : includes, + gnu_symbol_visibility : 'hidden', dependencies : alpm_deps) libalpm = library( -- 2.29.2
