[PATCH libICE] Enable visibility annotations

Thu Apr 28 14:28:31 UTC 2016

On 04/28/2016 04:59 PM, Emil Velikov wrote:
> 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/

I've checked them (from the very beginning actually) and there seems to 
be no strong requirement for attachments (it even says "Always attach 
patches as plain text files - if emailed then either attached or in-line.").

>>> 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.

But what would we do if there are problems with some 
compiler/version/platform (except for configure-time testing)?

> 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.

Note that _X_EXPORT is simply not defined for Cygwin and Mingw so there 
are basically no annotations at all. They ignore visibility annotations 
and warn on them by default:
   visi.c:0: warning: visibility attribute not supported in this 
configuration; ignored
Most OSS projects disable visibility on Cygwin/Mingw for this reason.

If we apply -fvisibility=hidden, this may result in all symbols in .dll 
being hidden (can't check right now though).

>>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
>
>