[Mesa-dev] [PATCH] configure.ac: Enable GLX_USE_TLS if possible.

[Dan Nicholson]

On Wed, Feb 8, 2012 at 5:11 PM, Zhigang Gong
<zhigang.gong at linux.intel.com> wrote:
> On Wed, Feb 08, 2012 at 02:46:42PM -0800, Eric Anholt wrote:
>> On Wed,  8 Feb 2012 16:19:38 +0800, zhigang.gong at linux.intel.com wrote:
>> > From: Zhigang Gong <zhigang.gong at linux.intel.com>
>> >
>> > If the system support tls, we prefer to enable it by default
>> > just as xserver does. Actually, the checking code is copied
>> > from xserver/configure.ac.
>> >
>> > Signed-off-by: Zhigang Gong <zhigang.gong at linux.intel.com>
>> > ---
>> >  configure.ac |   10 ++++++++--
>> >  1 files changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/configure.ac b/configure.ac
>> > index b2b1ab8..7c2756b 100644
>> > --- a/configure.ac
>> > +++ b/configure.ac
>> > @@ -1103,8 +1103,14 @@ dnl
>> >  AC_ARG_ENABLE([glx-tls],
>> >      [AS_HELP_STRING([--enable-glx-tls],
>> >          [enable TLS support in GLX @<:@default=disabled@:>@])],
>> > -    [GLX_USE_TLS="$enableval"],
>> > -    [GLX_USE_TLS=no])
>> > +    [GLX_USE_TLS=$enableval
>> > +     if test "x$GLX_USE_TLS" = "xyes" && test "${ac_cv_tls}" = "none" ; then
>> > +        AC_MSG_ERROR([GLX with TLS support requested, but the compiler does not support it.])
>> > +     fi],
>> > +    [GLX_USE_TLS=no
>> > +     if test "${ac_cv_tls}" != "none" ; then
>> > +        GLX_USE_TLS=yes
>> > +     fi])
>> >  AC_SUBST(GLX_TLS, ${GLX_USE_TLS})
>>
>> I don't think you have anything setting ac_cv_tls.
>
> You are right, I forgot to add the checking code which is also from xserver.
> Sorry for that, here is the v2 patch:
>
> From: Zhigang Gong <zhigang.gong at linux.intel.com>
> Date: Wed, 8 Feb 2012 16:12:42 +0800
> Subject: [PATCH v2] configure.ac: Enable GLX_USE_TLS if possible.
>
> If the system support tls, we prefer to enable it by default
> just as xserver does. Actually, the checking code is copied
> from xserver/configure.ac.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at linux.intel.com>
> ---
>  configure.ac |   39 +++++++++++++++++++++++++++++++++++++--
>  1 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index b2b1ab8..3dfafba 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1099,12 +1099,47 @@ esac
>  dnl
>  dnl TLS detection
>  dnl
> +AC_MSG_CHECKING(for thread local storage (TLS) support)
> +AC_CACHE_VAL(ac_cv_tls, [
> +    ac_cv_tls=none
> +    keywords="__thread __declspec(thread)"
> +    for kw in $keywords ; do
> +        AC_TRY_COMPILE([int $kw test;], [], ac_cv_tls=$kw)
> +    done
> +])
> +AC_MSG_RESULT($ac_cv_tls)
> +
> +if test "$ac_cv_tls" != "none"; then
> +    AC_MSG_CHECKING(for tls_model attribute support)
> +    AC_CACHE_VAL(ac_cv_tls_model, [
> +        save_CFLAGS="$CFLAGS"
> +        CFLAGS="$CFLAGS $STRICT_CFLAGS"
> +        AC_TRY_COMPILE([int $ac_cv_tls __attribute__((tls_model("initial-exec"))) test;], [],
> +                       ac_cv_tls_model=yes, ac_cv_tls_model=no)
> +        CFLAGS="$save_CFLAGS"
> +    ])
> +    AC_MSG_RESULT($ac_cv_tls_model)
> +
> +    if test "x$ac_cv_tls_model" = "xyes" ; then
> +        mesa_tls=$ac_cv_tls' __attribute__((tls_model("initial-exec")))'
> +    else
> +        mesa_tls=$ac_cv_tls
> +    fi
> +
> +    AC_DEFINE_UNQUOTED([TLS], $mesa_tls, [The compiler supported TLS storage class, prefering initial-exec if tls_model is supported])
> +fi

Can you put this code in acinclude.m4 and call it MESA_TLS or
something? There's a couple minor issues with copying this straight
from the xserver.

1. STRICT_FLAGS isn't set anywhere. This would come from
XORG_STRICT_OPTION, but I don't think we should pull that in here. I
think you can just drop the save_CFLAGS dance and just call
AC_TRY_COMPILE straight off. The main thing we're losing is possibly
having -Werror set during the run, but then you have to figure out if
that's OK to set or not. Seems easier to just see if the compiler will
bomb with whatever flags are set at the time.

2. Also, the AC_DEFINE_UNQUOTED is not needed since mesa doesn't use
config.h. If you want -DTLS in the code, I think you should add that
to DEFINES. Or make a new variable TLS_FLAGS and add it to CFLAGS in
configs/autoconf.in. Look at how PIC_FLAGS is used if you want this.
Probably this line should just be removed, though, since configure.ac
is already setting -DGLX_USE_TLS and that's used in the code.

>  AC_ARG_ENABLE([glx-tls],
>     [AS_HELP_STRING([--enable-glx-tls],
>         [enable TLS support in GLX @<:@default=disabled@:>@])],

You should change this to default:auto since the default case is
turning on TLS when the compiler says we have it.

> -    [GLX_USE_TLS="$enableval"],
> -    [GLX_USE_TLS=no])
> +    [GLX_USE_TLS=$enableval
> +     if test "x$GLX_USE_TLS" = "xyes" && test "${ac_cv_tls}" = "none" ; then
> +        AC_MSG_ERROR([GLX with TLS support requested, but the compiler does not support it.])
> +     fi],
> +    [GLX_USE_TLS=no
> +     if test "${ac_cv_tls}" != "none" ; then
> +        GLX_USE_TLS=yes
> +     fi])
>  AC_SUBST(GLX_TLS, ${GLX_USE_TLS})
>
>  AS_IF([test "x$GLX_USE_TLS" = xyes],

The rest looks good. With those couple fixes,

Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>