Re: [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)

[Christophe de Dinechin <christophe@xxxxxxxxxxxx>]

On 12 May 2017, at 15:29, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:

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 the
structure must be aligned, if not the code that allocated the structure
is broken, not container_of.
Wondering why this change is needed. Maybe code using ucontext is
causing it?

It’s another case that triggers the “cast changes alignment” warning. The correct fix is a SPICE_ALIGNED_CAST.

Christophe

Would that be better in your opinion?


Christophe

Regards,
Daniel

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel