[PATCH libICE] Enable visibility annotations

[Emil Velikov]

On 28 April 2016 at 15:28, Yury Gribov <y.gribov at samsung.com> wrote:
> 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.").
>
Looks like you read more than me - I never got past the first 15 lines
(the ones mentioning git send-mail). Sorry about that had no idea that
the docs are so ... interesting

>>> +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)?
>
Sort it out accordingly as we know a bit more about the users.

Which reminds me -> one should update the .pc file as well -> add
xproto to the Requires.private section.

>> 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.
>>
So the latter is needed. Fair enough.

>>> +    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.
>
Yes, as mentioned earlier, one should not be using attribute
visibility but __declspec dllexport/dllimport for Windows.
As to why they're missing is beyond me.

So all in all, even though some of my suggestions did not work out
there's still a few bits missing in the patch.

Thanks
Emil