[PATCH libICE] Enable visibility annotations

Fri May 6 09:27:44 UTC 2016

On 04/29/2016 06:49 PM, Yury Gribov wrote:
> 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).

Ping :)