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

[Frediano Ziglio]

> > On 12 May 2017, at 15:29, Frediano Ziglio < fziglio at redhat.com > wrote:
> 

> > > > On 11 May 2017, at 12:56, Daniel P. Berrange < berrange at redhat.com >
> > > > wrote:
> > > 
> > 
> 

> > > > On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote:
> > > 
> > 
> 

> > > > > From: Christophe de Dinechin < dinechin at redhat.com >
> > > > 
> > > 
> > 
> 

> > > > > Signed-off-by: Christophe de Dinechin < dinechin at redhat.com >
> > > > 
> > > 
> > 
> 
> > > > > ---
> > > > 
> > > 
> > 
> 
> > > > > 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.

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 

How does it sound? 

Frediano 

> Christophe

> > > Would that be better in your opinion?
> > 
> 

> > > Christophe
> > 
> 

> > > > Regards,
> > > 
> > 
> 
> > > > Daniel
> > > 
> > 
> 

> > Frediano
> 
> > _______________________________________________
> 
> > Spice-devel mailing list
> 
> > Spice-devel at lists.freedesktop.org
> 
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170515/10d64bd2/attachment-0001.html>