[PATCH libICE] Enable visibility annotations

Thu Apr 28 13:59:34 UTC 2016

On 27 April 2016 at 08:48, Yury Gribov <y.gribov at samsung.com> wrote:
> On 04/26/2016 02:14 PM, Emil Velikov wrote:
>>
>> On 26 April 2016 at 07:54, Yury Gribov <y.gribov at samsung.com> wrote:
>>>
>>> On 04/20/2016 10:03 AM, Yury Gribov wrote:
>>>>
>>>>
>>>> On 04/19/2016 06:35 PM, Yury Gribov wrote:
>>>>>
>>>>>
>>>>> On 04/18/2016 06:21 PM, Adam Jackson wrote:
>>>>>>
>>>>>>
>>>>>> On Mon, 2016-04-18 at 09:23 +0300, Yury Gribov wrote:
>>>>>>
>>>>>>> Does below look like a sane approach?
>>>>>>> 1) get all deps via
>>>>>>>      $ apt-cache rdepends libice6 libice-dev
>>>>>>> 2) unpack each of the above .debs and scan for ELFs
>>>>>>> 3) for each ELF see if one of now hidden symbols is UND
>>>>>>
>>>>>>
>>>>>>
>>>>>> That sounds good to me.
>>>>>
>>>>>
>>>>>
>>>>> I've cooked a simple script (attached, reviews welcome) and applied it
>>>>> to Ubuntu 14. It showed that there are additional uses for
>>>>> _IceTransNoListen (mentioned by Alan in separate email) but nothing
>>>>> else.
>>>>>
>>>>>>> Note that this does not handle transitive dependencies e.g. if some
>>>>>>> weird library links static version of libICE and the re-exports it's
>>>>>>> symbols as part of new lib's public interface.
>>>>>>
>>>>>>
>>>>>>
>>>>>> It'd be possible to scan for this too I suspect.  Look for packages
>>>>>> that BuildRequire libice6-static, scan the resulting binaries for
>>>>>> exports. I suspect there are zero such packages.
>>>>>
>>>>>
>>>>>
>>>>> I have postponed this activity for now (especially given that this
>>>>> behavior would be a serious packaging abuse).
>>>>
>>>>
>>>>
>>>> Here's an updated patch with added _IceTransNoListen.  Does this look
>>>> better now?
>>>
>>>
>>>
>>> Ping.
>>>
>> Pong. Sending patches as attachments is not the most productive way to
>> get things reviewed ;-)
>
>
> I see. Strangely the Content-Disposition in my previous email is properly
> set to "inline". I've both inlined and attached the new patch to be sure.
>
Close but no cigar :-P Please follow the suggestions in the wiki
http://www.x.org/wiki/Development/Documentation/SubmittingPatches/

>> But seriously, you want to update the configure.ac to handle more than
>> just GCC. Feel free to copy some/most of the logic from xserver's
>> configure.ac
>
>
> Right, fixed. I initially decided to not copy the visibility detection hunk
> from other places because was unsure of licenses and stuff. But xserver
> license is obviously compatible so should be fine.
>
>> Emil
>>
>>
>
>
> From 40fa9c6eb6fbaa924bf90efcefef681bbaab4194 Mon Sep 17 00:00:00 2001
> From: Yury Gribov <y.gribov at samsung.com>
> Date: Tue, 12 Apr 2016 17:04:09 +0300
> Subject: [PATCH] Added visibility annotations.
>
> Signed-off-by: Yury Gribov <y.gribov at samsung.com>
> ---
>  configure.ac              | 42 ++++++++++++++++++++++++
>  include/X11/ICE/ICElib.h  | 82
> +++++++++++++++++++++++------------------------
>  include/X11/ICE/ICEmsg.h  | 18 +++++------
>  include/X11/ICE/ICEutil.h | 18 +++++------
>  src/icetrans.c            |  4 +++
>  5 files changed, 105 insertions(+), 59 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 458882a..f63da07 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -29,8 +29,13 @@ XORG_WITH_FOP
>  XORG_WITH_XSLTPROC
>  XORG_CHECK_SGML_DOCTOOLS(1.8)
>
> +AC_ARG_ENABLE(visibility, AS_HELP_STRING([--enable-visibility], [Enable
> symbol visibility (default: auto)]),
> +              [SYMBOL_VISIBILITY=$enableval],
> +              [SYMBOL_VISIBILITY=auto])
> +
>  # Obtain compiler/linker options for depedencies
>  PKG_CHECK_MODULES(ICE, xproto xtrans)
> +PKG_PROG_PKG_CONFIG
>
>  # Transport selection macro from xtrans.m4
>  XTRANS_CONNECTION_FLAGS
> @@ -40,6 +45,43 @@ AC_DEFINE(ICE_t, 1, [Xtrans transport type])
>  AC_CHECK_LIB([bsd], [arc4random_buf])
>  AC_CHECK_FUNCS([asprintf arc4random_buf])
>
> +have_visibility=disabled
> +if test x$SYMBOL_VISIBILITY != xno; then
> +  AC_MSG_CHECKING(for symbol visibility support)
> +  if test x$GCC = xyes; then
> +    VISIBILITY_CFLAGS="-fvisibility=hidden"
> +  else
> +    if test x$SUNCC = xyes; then
> +      VISIBILITY_CFLAGS="-xldscope=hidden"
> +    else
> +      have_visibility=no
If Cygwin/Mingw does support the visibility flags (see below) I'd just
warn/error out in here.
IMHO if a specific platform/compiler is not handles we better get a
report and sort it out.

In general I would not bother with anything more than
 - PKG_CHECK_MODULE(XPROTO, xproto) + wire up the XPROTO_CFLAGS (both missing)
 - Add the above "if test $compiler set VISIBILILTY_FLAGS" + propagate
the latter throughout (both done)
 - Add (now missing) #include <X11/Xfuncproto.h> in the respective
places and annotate the functions (already done)

No extra configure toggles (above), no configure time testing. Unless
cygwin/mingw requires the latter.

> +    fi
> +  fi
> +  if test x$have_visibility != xno; then
> +    save_CFLAGS="$CFLAGS"
> +    proto_inc=`$PKG_CONFIG --cflags xproto`
> +    CFLAGS="$CFLAGS $VISIBILITY_CFLAGS $proto_inc"
> +    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
> +                                        [#include <X11/Xfuncproto.h>
> +                                         #if defined(__CYGWIN__) ||
> defined(__MINGW32__)
> +                                         #error No visibility support

Are you sure that think this is correct ? Afaict things should work,
despite that xserver sets SYMBOL_VISIBILITY=no.

Jon, any ideas why xserver disables -fvisibility=hidden ? Should we
remove that line or there's something subtle that requires the
behaviour ? For example: relying on symbols that are not marked as
exported or alike.

>From past (ancient) experiences and reading the wiki correctly,
combining -fivisiblity=hidden + dllimport/dllexport should work fine ?

Thanks
Emil

[1] https://gcc.gnu.org/wiki/Visibility