On 04/28/2016 04:59 PM, Emil Velikov wrote:
On 27 April 2016 at 08:48, Yury Gribov <[email protected]> wrote:
On 04/26/2016 02:14 PM, Emil Velikov wrote:
On 26 April 2016 at 07:54, Yury Gribov <[email protected]> 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.").
But seriously, you want to update the configure.ac to handle more than
just GCC. Feel free to copy some/most of the logic from xserver's
configure.ac
Right, fixed. I initially decided to not copy the visibility detection hunk
from other places because was unsure of licenses and stuff. But xserver
license is obviously compatible so should be fine.
Emil
From 40fa9c6eb6fbaa924bf90efcefef681bbaab4194 Mon Sep 17 00:00:00 2001
From: Yury Gribov <[email protected]>
Date: Tue, 12 Apr 2016 17:04:09 +0300
Subject: [PATCH] Added visibility annotations.
Signed-off-by: Yury Gribov <[email protected]>
---
configure.ac | 42 ++++++++++++++++++++++++
include/X11/ICE/ICElib.h | 82
+++++++++++++++++++++++------------------------
include/X11/ICE/ICEmsg.h | 18 +++++------
include/X11/ICE/ICEutil.h | 18 +++++------
src/icetrans.c | 4 +++
5 files changed, 105 insertions(+), 59 deletions(-)
diff --git a/configure.ac b/configure.ac
index 458882a..f63da07 100644
--- a/configure.ac
+++ b/configure.ac
@@ -29,8 +29,13 @@ XORG_WITH_FOP
XORG_WITH_XSLTPROC
XORG_CHECK_SGML_DOCTOOLS(1.8)
+AC_ARG_ENABLE(visibility, AS_HELP_STRING([--enable-visibility], [Enable
symbol visibility (default: auto)]),
+ [SYMBOL_VISIBILITY=$enableval],
+ [SYMBOL_VISIBILITY=auto])
+
# Obtain compiler/linker options for depedencies
PKG_CHECK_MODULES(ICE, xproto xtrans)
+PKG_PROG_PKG_CONFIG
# Transport selection macro from xtrans.m4
XTRANS_CONNECTION_FLAGS
@@ -40,6 +45,43 @@ AC_DEFINE(ICE_t, 1, [Xtrans transport type])
AC_CHECK_LIB([bsd], [arc4random_buf])
AC_CHECK_FUNCS([asprintf arc4random_buf])
+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)?
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.
+ 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.
If we apply -fvisibility=hidden, this may result in all symbols in .dll
being hidden (can't check right now though).
From past (ancient) experiences and reading the wiki correctly,
combining -fivisiblity=hidden + dllimport/dllexport should work fine ?
Thanks
Emil
[1] https://gcc.gnu.org/wiki/Visibility
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
