Re: [PATCHES] symbol visibility

[Marcel Holtmann <marcel holtmann org>]

Hi Dan,

> > > > while looking through the TODO list for 0.7 [1], I decided that I should 
> > > > contribute a bit, especially as it was me, that complained about the 
> > > > long list of exported symbols by libnm-glib and libnm-util.
> > > > 
> > > > So I went ahead and studied the docs a bit.
> > > > There are basically two approaches, how to trim down the list of 
> > > > exported symbols:
> > > > 
> > > > a) GCC's visibility support [2]
> > > >   GCC extension, mostly used by C++ libraries
> > > > 
> > > > b) ld linker/version scripts  [3]
> > > >   b.1) either directly via LDFLAGS -Wl,--version-script
> > > >   b.2) or indirectly via libtools -export-symbols $FILE or 
> > > > -export-symbols-regex, which will create a version script on the fly.
> > > > 
> > > > Before trying to evaluate which approach is best, I analyzed the list of 
> > > > exported symbols (see attached *.symbols files) and checked which 
> > > > symbols are not part of the public API (see attached *.diff files).
> > > > 
> > > > There are basically 5 ways
> > > > GCC visibility
> > > > 1.) Use -fvisibility=default (all symbols are exported by default) and 
> > > > augment all function declarations, which shall not be exported with 
> > > > __attribute__ ((visibility("hidden")))
> > > > 
> > > > Does not work, as we have autogenerated files, where we cannot add the 
> > > > visibility attribute.
> > > > 
> > > > 2.) Use -fvisibility=hidden (all symbols are hidden by default) and 
> > > > augment all function declarations, which shall be exported with 
> > > > __attribute__ ((visibility("default"))).
> > > > 
> > > > Lots of code changes in the header files. To be manageable we'd have to 
> > > > define a macro like DLLEXPORT. All installed header files would either 
> > > > have to define such a macro or we would have to create a new header file 
> > > > (like visibility.h) which contains this macro and all other header files 
> > > > include that. Imho quite cumbersome
> > > > 
> > > > libtool
> > > > 3.) -export-symbols-regex:
> > > > as you can see from the attached symbols and diff files, we can't use a 
> > > > common prefix like nm_.*. We'd have to rename private function to 
> > > > something like _nm_* -> looks of code changes.
> > > > 
> > > > 4.) -export-symbols $FILE
> > > > List all exported symbols in a separate file and pass it to libtool.
> > > > I chatted with libtool's upstream a bit, and he recommended though, to 
> > > > directly use a ld version script. as this provides the most flexibility,
> > > > 
> > > > 5.) LDFLAGS = -Wl,--version-script=$FILE
> > > > That's the way I implemented it in the patch. I listed each exported 
> > > > symbol explicitely (although the version script syntax would allow 
> > > > regexes). See patch 0001 and 0002 for the details.
> > > > Imho this is the nicest and cleanest approaches which requires the least 
> > > > changes.
> > > > I generated the version scripts automatically (by using objdump and 
> > > > grepping for the symbols in the installed header files, dropping all 
> > > > symbols which are not part of the public API), so a careful review would 
> > > > be very appreciated.
> > > > NOTE: I didn't enable symbol versioning as I didn't see the need for it 
> > > > yet, I only used the version script to control the list of exported symbols
> > > > 
> > > > After applying 0001 and 0002, the test-crypto binary will no longer 
> > > > build, as it uses private symbols from libnm-util. I simply removed it 
> > > > in patch 0003. An alternative would be, to add crypto.c and 
> > > > crypto_(gnutls|nss).c to test_crypto_SOURCES.
> > > > If someone has a need for this binary, I could change that.
> > > > 
> > > > 
> > > > Comments and feedback welcome.
> > > 
> > > I poked into visibility and talked to people who know stuff (ie Drepper)
> > > and the version script option seems to be most highly recommended one
> > > for a number of reasons.  So feel free to commit.
> > 
> > I ran into a similar problem with ConnMan a while ago and using libtool
> > for a binary doesn't seem to work. The versioning script is really the
> > best solution I could come up with. With some automake magic, I was able
> > to auto-generate the version file.
> > 
> > connmand_LDFLAGS = -Wl,--version-script=connman.ver
> > 
> > connmand_DEPENDENCIES = connman.ver
> > 
> > CLEANFILES = connman.ver connman.exp
> > 
> > connman.exp:
> > 	nm -B *.o | awk '{ print $$3 }' | sort -u | grep -E -e '^connman_' > $@
> > 
> > connman.ver: connman.exp
> > 	echo "{ global:" > $@
> > 	cat $< | sed -e "s/\(.*\)/\1;/" >> $@
> > 	echo "local: *; };" >> $@
> > 
> > So in this case, everything with the prefix connman_ will be made
> > public.
> 
> Good point.  Somebody did bring up the benefit that using a version
> script gives you, _at a glance_, your entire public API, which is a good
> thing.  Plus it would also ensure that we don't leak API by accident,
> though it does mean a maintenance overhead versus your solution here.

I forgot to mention that for internal functions I am using the __connman
prefix. Personally I prefer not to maintain two files with information
about exported symbols. However that is how I prefer it.

Regards

Marcel