[PATCH libICE] Enable visibility annotations
Yury Gribov
y.gribov at samsung.com
Fri May 6 11:18:00 UTC 2016
On 05/06/2016 01:54 PM, Emil Velikov wrote:
> 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.
Unfortunately I don't see any good approach. We could move definitions
of _X_EXPORT and friends to configure.ac but this would need to be done
for all X11 packages (not just libICE) which just does not scale.
>> 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
More information about the xorg-devel
mailing list