[PATCH libICE] Enable visibility annotations

Fri May 6 10:54:59 UTC 2016

Hi Yury,

On 6 May 2016 at 10:27, Yury Gribov <y.gribov at samsung.com> wrote:
> 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).
Sadly I don't see a way around this (barring MS getting their sh*t together).

If one warns/errors out in Xfuncproto.h then the build log will be
flooded/things won't build. On the other hand, if one drops the
configure check, then the logs will be flooded again. I think the
whole dislike comes to the point that we think that both
__attribute__((visibility("XX"))) and -fvisibility=foo are the same
thing. Sadly they are not - props to Alan for setting me straight.

If you can think of a way to simplify things please shout.

>
>
> Ping :)
>
So... it seems like I've lost my marbles with some of the suggestions.
Then again, I'm not sure if you're trolling me (re: the attachment),
considering my earlier suggestions.

To minimise the annoyance, I'll send an updated patch later on today.
I might also drop/rework the dubious hunk on the wiki.

Thanks
Emil