[PATCH libICE] Enable visibility annotations

Fri Apr 29 15:49:42 UTC 2016

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

Emil,

Please check if I've properly addressed all your suggestions.

I actually started to dislike the current X11 approach to visibility 
enabling. We seem to detect it twice - in package's configure.ac and in 
generic Xfuncproto.h, in totally different and probably sometimes 
incompatible ways (optimistic compile check in configure vs. rigid 
compiler version check in Xfuncproto.h).

-Y
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Added-visibility-annotations.patch
Type: text/x-patch
Size: 12620 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160429/de28293a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-xproto-dependency-private.patch
Type: text/x-patch
Size: 602 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160429/de28293a/attachment-0001.bin>