[Xcb] hidden-visibility and library size

[Jamey Sharp]

On Wed, Dec 28, 2005 at 12:14:46AM +0100, Vincent Torri wrote:
> Of course, we need to specify all the exported functions. There's an
> example in xcb.h.diff.

Oh, so that's how the visibility stuff works. OK. Thanks for putting
this together!

I think setting visibility hidden by default is the wrong choice here.
Instead of setting any compile flags or touching any public header
files, just fix xcbint.h so that everything there is marked either
hidden or internal. For XCB, internal should always be safe (we never
expose to external code pointers to internal functions) and buys us some
more speed and code size improvements.

> I've once tried to debug with -g -O2 a lib (evas, iirc) and gdb has
> given me odd values when i used bt f

Yes, that's normal. Debugging can be hard when combined with
optimization. But it's not impossible, and I do it on a regular basis,
especially with XCB where the optimizer doesn't seem to do much.

> my point of view is : when i debug my code, i don't want to debug the
> compiler optimizations. that's why I prefer to use only -g.

That's fine, but it's *your* preference: don't impose it on me. :-)

Here are some comments on your changes. I'd strongly prefer a single
patch in the form produced by `cvs diff -Nu`, by the way.

> --- Makefile.am.~1.26.~	2005-12-24 14:59:59.000000000 +0100
> +++ Makefile.am	2005-12-27 22:09:59.053154976 +0100
> @@ -41,8 +41,9 @@
>  xcbinclude_HEADERS = xcb.h xcbext.h xcbxlib.h $(COREHEADERS) $(EXTHEADERS)
>  
>  CFLAGS =
> -AM_CFLAGS = -include config.h $(CDEBUGFLAGS) $(XCBPROTO_CFLAGS) $(XPROTO_CFLAGS) $(XAU_CFLAGS)
> +AM_CFLAGS = -include config.h $(CDEBUGFLAGS) $(VISIBILITY_CFLAGS) $(XCBPROTO_CFLAGS) $(XPROTO_CFLAGS) $(XAU_CFLAGS)
>  libXCB_la_LIBADD = $(XCBPROTO_LIBS) $(XPROTO_LIBS) $(XAU_LIBS)
> +libXCB_la_LDFLAGS = -s

You've included your strip change in this patch; it should be cleaned up
before commit.

> dnl Detection and configuration of the visibility feature of gcc
> dnl Vincent Torri 2005-12-24
> dnl
> dnl AC_CHECK_VISIBILITY([ACTION-IF-FOUND [, ACTION-IF-NOT-FOUND]]])
> dnl Check the visibility feature of gcc and define VISIBILITY_CFLAGS
> dnl
> AC_DEFUN([AC_CHECK_VISIBILITY],
>    [VISIBILITY_CFLAGS=""
>     AC_MSG_CHECKING([whether ${CC} supports -fvisibility=hidden])
>     save_CFLAGS=${CFLAGS}
>     CFLAGS="$CFLAGS -fvisibility=hidden"
>     AC_TRY_LINK(
>        [],
>        [return 0;],
>        [VISIBILITY_CFLAGS="-fvisibility=hidden -DHAVE_VISIBILITY_FEATURE"
>         m4_if([$1], [], [:], [$1])],
>        [m4_if([$2], [], [:], [$2])])
>     AC_MSG_RESULT(${visibility})
>     CFLAGS=${save_CFLAGS}
>     AC_SUBST(VISIBILITY_CFLAGS)
>    ])

Maybe the macro should try compiling a function with the visibility
attribute instead of testing for -fvisibility?

It should probably use AC_COMPILE_IFELSE rather than AC_TRY_LINK.

I'd rather use
	AC_DEFINE(HAVE_VISIBILITY_FEATURE)
instead of passing a -D flag to the compiler.

> --- configure.ac	2005-12-27 22:53:31.313031112 +0100
> +++ configure.ac.new	2005-12-27 22:53:14.602571488 +0100
> @@ -5,7 +5,9 @@
>  AC_INIT([libXCB],
>          0.9,
>          [xcb at lists.freedesktop.org])
> +
>  AC_CONFIG_SRCDIR([xcb.pc.in])
> +

Please don't include whitespace patches like this.

> @@ -78,4 +80,29 @@
>  	-Wstrict-prototypes -Wmissing-declarations -Wnested-externs"
>  AC_SUBST(CDEBUGFLAGS)
>  
> -AC_OUTPUT([Makefile src/Makefile tests/Makefile xcb.pc])
> +AC_CHECK_VISIBILITY([has_visibility="yes"], [has_visibility="no"])
> +
> +AC_CONFIG_FILES([Makefile src/Makefile tests/Makefile])
> +AC_CONFIG_FILES([xcb.pc])
> +
> +AC_OUTPUT

I don't understand. Why did you change the AC_OUTPUT invocation to use
AC_CONFIG_FILES? If it's important, please post a separate patch for it.

> +
> +#####################################################################
> +## Info
> +
> +echo
> +echo "------------------------------------------------------------------------"
> +echo " $PACKAGE $VERSION"
> +echo "------------------------------------------------------------------------"
> +echo
> +echo " Visibility feature..: $has_visibility"
> +echo " Optimisation flags..: $optflags"
> +echo
> +echo " Install path........: $prefix"
> +echo "   binaries..........: $bindir"
> +echo "   libraries.........: ${libdir}"
> +echo "   headers...........: ${xcbincludedir}"
> +echo
> +echo " Compilation.........: make"
> +echo " Installation........: make install"
> +echo

Obviously this change shouldn't be part of this patch. I'm not sure I
want it committed at all, though I don't feel strongly about that.

--Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/xcb/attachments/20051227/04ea4de9/attachment-0001.pgp