Re: [PATCHES] symbol visibility

[Dan Williams <dcbw redhat com>]

On Fri, 2008-08-15 at 22:07 +0200, Marcel Holtmann wrote:
> 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.

Dan

> If the versioning script would handle regular expression by itself, then
> that would be a lot simpler actually. At least I couldn't get that to
> work.
> 
> Regards
> 
> Marcel
> 
>