On 11 May 2017, at 12:56, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:
On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote:
From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
---
configure.ac | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/configure.ac b/configure.ac
index 74b5811..ecab365 100644
--- a/configure.ac
+++ b/configure.ac
@@ -62,6 +62,18 @@ esac
AC_MSG_RESULT([$os_win32])
AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"])
+AC_MSG_CHECKING([for native macOS])
+case "$host_os" in
+ *darwin*)
+ os_mac=yes
+ ;;
+ *)
+ os_mac=no
+ ;;
+esac
+AC_MSG_RESULT([$os_mac])
+AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"])
+
AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h])
AC_CHECK_HEADERS([termios.h])
AC_CHECK_HEADERS([epoxy/egl.h],
@@ -468,6 +480,8 @@ esac
if test "$with_coroutine" = "auto"; then
if test "$os_win32" = "yes"; then
with_coroutine=winfiber
+ elif test "$os_mac" = "yes"; then
+ with_coroutine=gthread
else
with_coroutine=ucontext
fi
Despite ucontext being deprecated we are still better off using that &
ignoring the warnings, than using the gthread impl.
Yes, I remember you explained the benefits of keeping ucontext. But for the
moment at least, on macOS, it is not a warning:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/ucontext.h:43:2:
error:
The deprecated ucontext routines require _XOPEN_SOURCE to be defined
#error The deprecated ucontext routines require _XOPEN_SOURCE to be defined
^
1 error generated.
So I can:
- Add the error in the patch description
- Attempt to define _XOPEN_SOURCE in the configuration. I’m concerned about
side effects.
The latter leads to another can of worms. Notably, the macro container_of
triggers the alignment warning for container_of, so I have a set of
alignment warnings, and a set of deprecation warnings. But it builds. The
incremental patch would be something like:
diff --git a/configure.ac b/configure.ac
index ecab365..8b433ba 100644
--- a/configure.ac
+++ b/configure.ac
@@ -481,7 +481,8 @@ if test "$with_coroutine" = "auto"; then
if test "$os_win32" = "yes"; then
with_coroutine=winfiber
elif test "$os_mac" = "yes"; then
- with_coroutine=gthread
+ with_coroutine=ucontext
+ AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for
ucontext])
else
with_coroutine=ucontext
fi
diff --git a/src/continuation.h b/src/continuation.h
index 675a257..cbca06e 100644
--- a/src/continuation.h
+++ b/src/continuation.h
@@ -49,7 +49,7 @@ int cc_swap(struct continuation *from, struct continuation
*to);
#define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
#define container_of(obj, type, member) \
- (type *)(((char *)obj) - offset_of(type, member))
+ (type *)(void *)(((char *)obj) - offset_of(type, member))
#endif
/*
This change is ok. If it was a member of a structure surely thestructure must be aligned, if not the code that allocated the structureis broken, not container_of.Wondering why this change is needed. Maybe code using ucontext iscausing it?
It’s another case that triggers the “cast changes alignment” warning. The correct fix is a SPICE_ALIGNED_CAST.
I would go then with 3 macros instead of two
SPICE_ALIGNED_CAST, SPICE_UNALIGNED_CAST. We know if is aligned or not and we just make the cast, no additional checks.
SPICE_ALIGNED_CHECK_CAST. We are not sure and we do the check giving warning
What are the cases where you are sure enough that it’s aligned to not even check? If the compiler does its inlining job properly, the test is a bitmask test witih a constant, so typically one instruction unless it fails. I don’t think it’s really worth having a non-checking variant. Maybe I could add a G_UNLIKELY?
Christophe
|