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

[Christophe de Dinechin]

> On 15 May 2017, at 11:58, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> 
> On 12 May 2017, at 15:29, Frediano Ziglio <fziglio at redhat.com <mailto:fziglio at redhat.com>> wrote:
> 
> 
> On 11 May 2017, at 12:56, Daniel P. Berrange <berrange at redhat.com <mailto: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 <mailto:dinechin at redhat.com>>
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com <mailto: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

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

> 
> 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 <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
> 
> 
> _______________________________________________
> 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/a5a02d8b/attachment.html>