[pacman-dev,1/3] meson: replace objects with link_whole

Message ID 20201223224224.1434521-1-emil.l.velikov@gmail.com
State New
Headers show
Series [pacman-dev,1/3] meson: replace objects with link_whole | expand

Commit Message

Emil Velikov Dec. 23, 2020, 10:42 p.m. UTC
Instead extracting/listing objects from existing static libraries,
simply add those to the link list. Note that link_whole is needed
otherwise, unused code will be pruned.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 meson.build | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Emil Velikov Dec. 24, 2020, 4:05 p.m. UTC | #1
On Thu, 24 Dec 2020 at 01:38, Eli Schwartz <eschwartz@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
Allan McRae Dec. 29, 2020, 3:15 a.m. UTC | #2
On 25/12/20 2:05 am, Emil Velikov wrote:
> On Thu, 24 Dec 2020 at 01:38, Eli Schwartz <eschwartz@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
Eli Schwartz Dec. 29, 2020, 3:47 a.m. UTC | #3
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 <eschwartz@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...

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.

Patch

diff --git a/meson.build b/meson.build
index 264c6501..fd573826 100644
--- a/meson.build
+++ b/meson.build
@@ -310,15 +310,14 @@  alpm_deps = [crypto_provider, libarchive, libcurl, libintl, gpgme]
 libalpm_a = static_library(
   'alpm_objlib',
   libalpm_sources,
-  # https://github.com/mesonbuild/meson/issues/3937
-  objects : libcommon.extract_all_objects(),
+  link_whole: libcommon,
   include_directories : includes,
   dependencies : alpm_deps)
 
 libalpm = library(
   'alpm',
   version : libalpm_version,
-  objects: libalpm_a.extract_all_objects(recursive: true),
+  link_whole: libalpm_a,
   dependencies : alpm_deps,
   install : true)