On 04/29/2016 04:28 PM, Emil Velikov wrote:
On 28 April 2016 at 15:28, Yury Gribov <[email protected]> wrote:
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.").
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
>From ceee5f97622407ec2cbcaa8c4883428b9e9d89dd Mon Sep 17 00:00:00 2001
From: Yury Gribov <[email protected]>
Date: Tue, 12 Apr 2016 17:04:09 +0300
Subject: [PATCH 2/2] Added visibility annotations.
Signed-off-by: Yury Gribov <[email protected]>
---
configure.ac | 31 ++++++++++++++++++
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, 94 insertions(+), 59 deletions(-)
diff --git a/configure.ac b/configure.ac
index 458882a..ff5d4c8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -31,6 +31,7 @@ XORG_CHECK_SGML_DOCTOOLS(1.8)
# Obtain compiler/linker options for depedencies
PKG_CHECK_MODULES(ICE, xproto xtrans)
+PKG_CHECK_MODULES(XPROTO, xproto)
# Transport selection macro from xtrans.m4
XTRANS_CONNECTION_FLAGS
@@ -40,6 +41,36 @@ AC_DEFINE(ICE_t, 1, [Xtrans transport type])
AC_CHECK_LIB([bsd], [arc4random_buf])
AC_CHECK_FUNCS([asprintf arc4random_buf])
+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
+ fi
+fi
+save_CFLAGS="$CFLAGS"
+CFLAGS="$CFLAGS $VISIBILITY_CFLAGS $PROTO_CFLAGS"
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
+ [#include <X11/Xfuncproto.h>
+ #if defined(__CYGWIN__) || defined(__MINGW32__)
+ #error No visibility support
+ #endif
+ extern _X_HIDDEN int hidden_int;
+ extern _X_EXPORT int public_int;
+ extern _X_HIDDEN int hidden_int_func(void);
+ extern _X_EXPORT int public_int_func(void);]],
+ [])],
+ have_visibility=yes,
+ have_visibility=no)
+CFLAGS=$save_CFLAGS
+AC_MSG_RESULT([$have_visibility])
+if test x$have_visibility != xno; then
+ ICE_CFLAGS="$ICE_CFLAGS $VISIBILITY_CFLAGS"
+fi
+
# Allow checking code with lint, sparse, etc.
XORG_WITH_LINT
XORG_LINT_LIBRARY([ICE])
diff --git a/include/X11/ICE/ICElib.h b/include/X11/ICE/ICElib.h
index 402cbc8..a1d2256 100644
--- a/include/X11/ICE/ICElib.h
+++ b/include/X11/ICE/ICElib.h
@@ -205,7 +205,7 @@ typedef void (*IceIOErrorHandler) (
_XFUNCPROTOBEGIN
-extern int IceRegisterForProtocolSetup (
+_X_EXPORT int IceRegisterForProtocolSetup (
const char * /* protocolName */,
const char * /* vendor */,
const char * /* release */,
@@ -217,7 +217,7 @@ extern int IceRegisterForProtocolSetup (
IceIOErrorProc /* IOErrorProc */
);
-extern int IceRegisterForProtocolReply (
+_X_EXPORT int IceRegisterForProtocolReply (
const char * /* protocolName */,
const char * /* vendor */,
const char * /* release */,
@@ -232,7 +232,7 @@ extern int IceRegisterForProtocolReply (
IceIOErrorProc /* IOErrorProc */
);
-extern IceConn IceOpenConnection (
+_X_EXPORT IceConn IceOpenConnection (
char * /* networkIdsList */,
IcePointer /* context */,
Bool /* mustAuthenticate */,
@@ -241,18 +241,18 @@ extern IceConn IceOpenConnection (
char * /* errorStringRet */
);
-extern IcePointer IceGetConnectionContext (
+_X_EXPORT IcePointer IceGetConnectionContext (
IceConn /* iceConn */
);
-extern Status IceListenForConnections (
+_X_EXPORT Status IceListenForConnections (
int * /* countRet */,
IceListenObj ** /* listenObjsRet */,
int /* errorLength */,
char * /* errorStringRet */
);
-extern Status IceListenForWellKnownConnections (
+_X_EXPORT Status IceListenForWellKnownConnections (
char * /* port */,
int * /* countRet */,
IceListenObj ** /* listenObjsRet */,
@@ -260,58 +260,58 @@ extern Status IceListenForWellKnownConnections (
char * /* errorStringRet */
);
-extern int IceGetListenConnectionNumber (
+_X_EXPORT int IceGetListenConnectionNumber (
IceListenObj /* listenObj */
);
-extern char *IceGetListenConnectionString (
+_X_EXPORT char *IceGetListenConnectionString (
IceListenObj /* listenObj */
);
-extern char *IceComposeNetworkIdList (
+_X_EXPORT char *IceComposeNetworkIdList (
int /* count */,
IceListenObj * /* listenObjs */
);
-extern void IceFreeListenObjs (
+_X_EXPORT void IceFreeListenObjs (
int /* count */,
IceListenObj * /* listenObjs */
);
-extern void IceSetHostBasedAuthProc (
+_X_EXPORT void IceSetHostBasedAuthProc (
IceListenObj /* listenObj */,
IceHostBasedAuthProc /* hostBasedAuthProc */
);
-extern IceConn IceAcceptConnection (
+_X_EXPORT IceConn IceAcceptConnection (
IceListenObj /* listenObj */,
IceAcceptStatus * /* statusRet */
);
-extern void IceSetShutdownNegotiation (
+_X_EXPORT void IceSetShutdownNegotiation (
IceConn /* iceConn */,
Bool /* negotiate */
);
-extern Bool IceCheckShutdownNegotiation (
+_X_EXPORT Bool IceCheckShutdownNegotiation (
IceConn /* iceConn */
);
-extern IceCloseStatus IceCloseConnection (
+_X_EXPORT IceCloseStatus IceCloseConnection (
IceConn /* iceConn */
);
-extern Status IceAddConnectionWatch (
+_X_EXPORT Status IceAddConnectionWatch (
IceWatchProc /* watchProc */,
IcePointer /* clientData */
);
-extern void IceRemoveConnectionWatch (
+_X_EXPORT void IceRemoveConnectionWatch (
IceWatchProc /* watchProc */,
IcePointer /* clientData */
);
-extern IceProtocolSetupStatus IceProtocolSetup (
+_X_EXPORT IceProtocolSetupStatus IceProtocolSetup (
IceConn /* iceConn */,
int /* myOpcode */,
IcePointer /* clientData */,
@@ -324,89 +324,89 @@ extern IceProtocolSetupStatus IceProtocolSetup (
char * /* errorStringRet */
);
-extern Status IceProtocolShutdown (
+_X_EXPORT Status IceProtocolShutdown (
IceConn /* iceConn */,
int /* majorOpcode */
);
-extern IceProcessMessagesStatus IceProcessMessages (
+_X_EXPORT IceProcessMessagesStatus IceProcessMessages (
IceConn /* iceConn */,
IceReplyWaitInfo * /* replyWait */,
Bool * /* replyReadyRet */
);
-extern Status IcePing (
+_X_EXPORT Status IcePing (
IceConn /* iceConn */,
IcePingReplyProc /* pingReplyProc */,
IcePointer /* clientData */
);
-extern char *IceAllocScratch (
+_X_EXPORT char *IceAllocScratch (
IceConn /* iceConn */,
unsigned long /* size */
);
-extern int IceFlush (
+_X_EXPORT int IceFlush (
IceConn /* iceConn */
);
-extern int IceGetOutBufSize (
+_X_EXPORT int IceGetOutBufSize (
IceConn /* iceConn */
);
-extern int IceGetInBufSize (
+_X_EXPORT int IceGetInBufSize (
IceConn /* iceConn */
);
-extern IceConnectStatus IceConnectionStatus (
+_X_EXPORT IceConnectStatus IceConnectionStatus (
IceConn /* iceConn */
);
-extern char *IceVendor (
+_X_EXPORT char *IceVendor (
IceConn /* iceConn */
);
-extern char *IceRelease (
+_X_EXPORT char *IceRelease (
IceConn /* iceConn */
);
-extern int IceProtocolVersion (
+_X_EXPORT int IceProtocolVersion (
IceConn /* iceConn */
);
-extern int IceProtocolRevision (
+_X_EXPORT int IceProtocolRevision (
IceConn /* iceConn */
);
-extern int IceConnectionNumber (
+_X_EXPORT int IceConnectionNumber (
IceConn /* iceConn */
);
-extern char *IceConnectionString (
+_X_EXPORT char *IceConnectionString (
IceConn /* iceConn */
);
-extern unsigned long IceLastSentSequenceNumber (
+_X_EXPORT unsigned long IceLastSentSequenceNumber (
IceConn /* iceConn */
);
-extern unsigned long IceLastReceivedSequenceNumber (
+_X_EXPORT unsigned long IceLastReceivedSequenceNumber (
IceConn /* iceConn */
);
-extern Bool IceSwapping (
+_X_EXPORT Bool IceSwapping (
IceConn /* iceConn */
);
-extern IceErrorHandler IceSetErrorHandler (
+_X_EXPORT IceErrorHandler IceSetErrorHandler (
IceErrorHandler /* handler */
);
-extern IceIOErrorHandler IceSetIOErrorHandler (
+_X_EXPORT IceIOErrorHandler IceSetIOErrorHandler (
IceIOErrorHandler /* handler */
);
-extern char *IceGetPeerName (
+_X_EXPORT char *IceGetPeerName (
IceConn /* iceConn */
);
@@ -414,15 +414,15 @@ extern char *IceGetPeerName (
* Multithread Routines
*/
-extern Status IceInitThreads (
+_X_EXPORT Status IceInitThreads (
void
);
-extern void IceAppLockConn (
+_X_EXPORT void IceAppLockConn (
IceConn /* iceConn */
);
-extern void IceAppUnlockConn (
+_X_EXPORT void IceAppUnlockConn (
IceConn /* iceConn */
);
diff --git a/include/X11/ICE/ICEmsg.h b/include/X11/ICE/ICEmsg.h
index f6e7121..d2636aa 100644
--- a/include/X11/ICE/ICEmsg.h
+++ b/include/X11/ICE/ICEmsg.h
@@ -39,46 +39,46 @@ _XFUNCPROTOBEGIN
* Function prototypes for internal ICElib functions
*/
-extern Status _IceRead (
+_X_EXPORT Status _IceRead (
IceConn /* iceConn */,
unsigned long /* nbytes */,
char * /* ptr */
);
-extern void _IceReadSkip (
+_X_EXPORT void _IceReadSkip (
IceConn /* iceConn */,
unsigned long /* nbytes */
);
-extern void _IceWrite (
+_X_EXPORT void _IceWrite (
IceConn /* iceConn */,
unsigned long /* nbytes */,
char * /* ptr */
);
-extern void _IceErrorBadMinor (
+_X_EXPORT void _IceErrorBadMinor (
IceConn /* iceConn */,
int /* majorOpcode */,
int /* offendingMinor */,
int /* severity */
);
-extern void _IceErrorBadState (
+_X_EXPORT void _IceErrorBadState (
IceConn /* iceConn */,
int /* majorOpcode */,
int /* offendingMinor */,
int /* severity */
);
-extern void _IceErrorBadLength (
+_X_EXPORT void _IceErrorBadLength (
IceConn /* iceConn */,
int /* majorOpcode */,
int /* offendingMinor */,
int /* severity */
);
-extern void _IceErrorBadValue (
+_X_EXPORT void _IceErrorBadValue (
IceConn /* iceConn */,
int /* majorOpcode */,
int /* offendingMinor */,
@@ -87,7 +87,7 @@ extern void _IceErrorBadValue (
IcePointer /* value */
);
-extern IcePoAuthStatus _IcePoMagicCookie1Proc (
+_X_EXPORT IcePoAuthStatus _IcePoMagicCookie1Proc (
IceConn /* iceConn */,
IcePointer * /* authStatePtr */,
Bool /* cleanUp */,
@@ -99,7 +99,7 @@ extern IcePoAuthStatus _IcePoMagicCookie1Proc (
char ** /* errorStringRet */
);
-extern IcePaAuthStatus _IcePaMagicCookie1Proc (
+_X_EXPORT IcePaAuthStatus _IcePaMagicCookie1Proc (
IceConn /* iceConn */,
IcePointer * /* authStatePtr */,
Bool /* swap */,
diff --git a/include/X11/ICE/ICEutil.h b/include/X11/ICE/ICEutil.h
index dbf1490..592993e 100644
--- a/include/X11/ICE/ICEutil.h
+++ b/include/X11/ICE/ICEutil.h
@@ -76,45 +76,45 @@ typedef struct {
* Function Prototypes
*/
-extern char *IceAuthFileName (
+_X_EXPORT char *IceAuthFileName (
void
);
-extern int IceLockAuthFile (
+_X_EXPORT int IceLockAuthFile (
const char * /* file_name */,
int /* retries */,
int /* timeout */,
long /* dead */
);
-extern void IceUnlockAuthFile (
+_X_EXPORT void IceUnlockAuthFile (
const char * /* file_name */
);
-extern IceAuthFileEntry *IceReadAuthFileEntry (
+_X_EXPORT IceAuthFileEntry *IceReadAuthFileEntry (
FILE * /* auth_file */
);
-extern void IceFreeAuthFileEntry (
+_X_EXPORT void IceFreeAuthFileEntry (
IceAuthFileEntry * /* auth */
);
-extern Status IceWriteAuthFileEntry (
+_X_EXPORT Status IceWriteAuthFileEntry (
FILE * /* auth_file */,
IceAuthFileEntry * /* auth */
);
-extern IceAuthFileEntry *IceGetAuthFileEntry (
+_X_EXPORT IceAuthFileEntry *IceGetAuthFileEntry (
const char * /* protocol_name */,
const char * /* network_id */,
const char * /* auth_name */
);
-extern char *IceGenerateMagicCookie (
+_X_EXPORT char *IceGenerateMagicCookie (
int /* len */
);
-extern void IceSetPaAuthData (
+_X_EXPORT void IceSetPaAuthData (
int /* numEntries */,
IceAuthDataEntry * /* entries */
);
diff --git a/src/icetrans.c b/src/icetrans.c
index 52e432b..8005aba 100644
--- a/src/icetrans.c
+++ b/src/icetrans.c
@@ -24,8 +24,12 @@
#include <config.h>
#endif
+#include <X11/Xfuncproto.h>
+
#define ICE_t 1
#define TRANS_CLIENT 1
#define TRANS_SERVER 1
+_X_EXPORT int _IceTransNoListen (const char * protocol);
+
#include <X11/Xtrans/transport.c>
--
1.9.1
>From 8ed25cd594a7f586308fbb67f93b116d392c21fd Mon Sep 17 00:00:00 2001
From: Yury Gribov <[email protected]>
Date: Fri, 29 Apr 2016 18:42:33 +0300
Subject: [PATCH 1/2] Make xproto dependency private.
---
ice.pc.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ice.pc.in b/ice.pc.in
index b96c9f8..1d0a4de 100644
--- a/ice.pc.in
+++ b/ice.pc.in
@@ -6,6 +6,6 @@ includedir=@includedir@
Name: ICE
Description: X Inter Client Exchange Library
Version: @PACKAGE_VERSION@
-Requires: xproto
+Requires.private: xproto
Cflags: -I${includedir}
Libs: -L${libdir} -lICE
--
1.9.1
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
