[Mesa-dev] gallium/util: Define ffsll on OpenBSD.

Submitted by Jon Turney on Feb. 9, 2015, 4:59 p.m.

Details

Message ID 54D8E75A.5090305@dronecode.org.uk
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Jon Turney Feb. 9, 2015, 4:59 p.m.
On 06/02/2015 19:58, Matt Turner wrote:
> On Fri, Feb 6, 2015 at 3:38 AM, Jonathan Gray <jsg@jsg.id.au> wrote:
>> OpenBSD has ffs in libc but does not have ffsll so use the compiler
>> builtin.  PIPE_OS_BSD isn't suitable here as FreeBSD has ffsll in libc.
>>
>> Signed-off-by: Jonathan Gray <jsg@jsg.id.au>
>> ---
>>   src/gallium/auxiliary/util/u_math.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/util/u_math.h b/src/gallium/auxiliary/util/u_math.h
>> index 5db5b66..ec282f3 100644
>> --- a/src/gallium/auxiliary/util/u_math.h
>> +++ b/src/gallium/auxiliary/util/u_math.h
>> @@ -531,6 +531,8 @@ unsigned ffs( unsigned u )
>>   #elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID)
>>   #define ffs __builtin_ffs
>>   #define ffsll __builtin_ffsll
>> +#elif defined(__OpenBSD__)
>> +#define ffsll __builtin_ffsll
>>   #endif
>
> Autoconf checks for presence of a bunch of builtins. Please use those
> instead (in this case, HAVE___BUILTIN_FFSLL).

Yes, please.

This has just been 'fixed' for MinGW, now for OpenBSD, and also needs 
fixing for Cygwin.

Attached is a patch which attempts to do this using autoconf checks.
From 580eb16295a94012c488db7ac44d09cb3ca8ff55 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Sat, 7 Feb 2015 14:17:35 +0000
Subject: [PATCH] gallium/util: Use autoconf check for ffs() and ffsll()

u_math.h should probably explicitly include config.h, but it didn't before and
already relies on some HAVE_ defines, so I haven't added it.

This makes a subtle change on non-linux platforms: previously we were explicitly
using __builtin_ffs(), whereas now we are using ffs(), and the compiler might
decide to convert that to a builtin.  If we wanted to ensure the builtin was
used for some reason, this isn't quite right.

Since ffs() is used, we need to include <strings.h> for the prototype, which I
think we expect to find on all POSIX systems, but not on MSVC.  Add an autoconf
check for that header and use that to conditionalize it's use.

Define _GNU_SOURCE on Cygwin so that the GNU extension ffsll() is prototyped.

I think I've put the right defines in the right place for the Android build
system, but I haven't build tested that.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 Android.common.mk                   |  3 +++
 configure.ac                        |  6 +++++-
 src/gallium/auxiliary/util/u_math.h | 12 ++++++++++--
 3 files changed, 18 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/Android.common.mk b/Android.common.mk
index 3e6d4c3..7719096 100644
--- a/Android.common.mk
+++ b/Android.common.mk
@@ -42,6 +42,9 @@  LOCAL_CFLAGS += \
 
 LOCAL_CFLAGS += \
 	-DHAVE_PTHREAD=1 \
+	-DHAVE_STRINGS_H \
+	-DHAVE___BUILTIN_FFS \
+	-DHAVE___BUILTIN_FFSLL \
 	-fvisibility=hidden \
 	-Wno-sign-compare
 
diff --git a/configure.ac b/configure.ac
index 7c2692e..cd4d7ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -172,6 +172,10 @@  if test "x$GCC" = xyes -a "x$acv_mesa_CLANG" = xno; then
     fi
 fi
 
+dnl Check for ffs and ffsll
+AC_CHECK_HEADER([strings.h])
+AC_CHECK_FUNCS([ffs ffsll])
+
 dnl Check for compiler builtins
 AX_GCC_BUILTIN([__builtin_bswap32])
 AX_GCC_BUILTIN([__builtin_bswap64])
@@ -219,7 +223,7 @@  solaris*)
     DEFINES="$DEFINES -DSVR4"
     ;;
 cygwin*)
-    DEFINES="$DEFINES -D_XOPEN_SOURCE=700"
+    DEFINES="$DEFINES -D_GNU_SOURCE"
     ;;
 esac
 
diff --git a/src/gallium/auxiliary/util/u_math.h b/src/gallium/auxiliary/util/u_math.h
index 5db5b66..45587fb 100644
--- a/src/gallium/auxiliary/util/u_math.h
+++ b/src/gallium/auxiliary/util/u_math.h
@@ -52,7 +52,7 @@  extern "C" {
 #include <float.h>
 #include <stdarg.h>
 
-#ifdef PIPE_OS_UNIX
+#ifdef HAVE_STRINGS_H
 #include <strings.h> /* for ffs */
 #endif
 
@@ -528,10 +528,18 @@  unsigned ffs( unsigned u )
 
    return i;
 }
-#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID)
+#else
+#ifndef HAVE_FFS
+#ifdef HAVE___BUILTIN_FFS
 #define ffs __builtin_ffs
+#endif
+#endif
+#ifndef HAVE_FFSLL
+#ifdef HAVE___BUILTIN_FFSLL
 #define ffsll __builtin_ffsll
 #endif
+#endif
+#endif
 
 #endif /* FFS_DEFINED */
 

Comments

On 09/02/15 16:59, Jon TURNEY wrote:
> On 06/02/2015 19:58, Matt Turner wrote:
>> On Fri, Feb 6, 2015 at 3:38 AM, Jonathan Gray <jsg@jsg.id.au> wrote:
>>> OpenBSD has ffs in libc but does not have ffsll so use the compiler
>>> builtin.  PIPE_OS_BSD isn't suitable here as FreeBSD has ffsll in libc.
>>>
>>> Signed-off-by: Jonathan Gray <jsg@jsg.id.au>
>>> ---
>>>   src/gallium/auxiliary/util/u_math.h | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/gallium/auxiliary/util/u_math.h
>>> b/src/gallium/auxiliary/util/u_math.h
>>> index 5db5b66..ec282f3 100644
>>> --- a/src/gallium/auxiliary/util/u_math.h
>>> +++ b/src/gallium/auxiliary/util/u_math.h
>>> @@ -531,6 +531,8 @@ unsigned ffs( unsigned u )
>>>   #elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID)
>>>   #define ffs __builtin_ffs
>>>   #define ffsll __builtin_ffsll
>>> +#elif defined(__OpenBSD__)
>>> +#define ffsll __builtin_ffsll
>>>   #endif
>>
>> Autoconf checks for presence of a bunch of builtins. Please use those
>> instead (in this case, HAVE___BUILTIN_FFSLL).
>
> Yes, please.
>
> This has just been 'fixed' for MinGW, now for OpenBSD, and also needs
> fixing for Cygwin.

>
> Attached is a patch which attempts to do this using autoconf checks.

The issue is that this will break scons builds unless these checks are 
replicated there.  And SCons implementation of configure checks are not 
great to be honest -- they either are cached (but in such way were 
multiple builds from same source tree pick up wrong values) or they need 
to be re-checked on every build (wasting time for incremental builds).

This is why, within reason, I personally like to avoid configure checks 
when practical.


So for now I'd prefer to leave MinGW 'fixed' as you put it.

But fell free to fix the other platforms as you propose.


BTW, isn't there any standard include that defines ffsll as macro or 
inline on top of __builtin_ffsll for systems that support it?


Jose
On Mon, Feb 09, 2015 at 07:22:35PM +0000, Jose Fonseca wrote:
> On 09/02/15 16:59, Jon TURNEY wrote:
> >On 06/02/2015 19:58, Matt Turner wrote:
> >>On Fri, Feb 6, 2015 at 3:38 AM, Jonathan Gray <jsg@jsg.id.au> wrote:
> >>>OpenBSD has ffs in libc but does not have ffsll so use the compiler
> >>>builtin.  PIPE_OS_BSD isn't suitable here as FreeBSD has ffsll in libc.
> >>>
> >>>Signed-off-by: Jonathan Gray <jsg@jsg.id.au>
> >>>---
> >>>  src/gallium/auxiliary/util/u_math.h | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>>diff --git a/src/gallium/auxiliary/util/u_math.h
> >>>b/src/gallium/auxiliary/util/u_math.h
> >>>index 5db5b66..ec282f3 100644
> >>>--- a/src/gallium/auxiliary/util/u_math.h
> >>>+++ b/src/gallium/auxiliary/util/u_math.h
> >>>@@ -531,6 +531,8 @@ unsigned ffs( unsigned u )
> >>>  #elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID)
> >>>  #define ffs __builtin_ffs
> >>>  #define ffsll __builtin_ffsll
> >>>+#elif defined(__OpenBSD__)
> >>>+#define ffsll __builtin_ffsll
> >>>  #endif
> >>
> >>Autoconf checks for presence of a bunch of builtins. Please use those
> >>instead (in this case, HAVE___BUILTIN_FFSLL).
> >
> >Yes, please.
> >
> >This has just been 'fixed' for MinGW, now for OpenBSD, and also needs
> >fixing for Cygwin.
> 
> >
> >Attached is a patch which attempts to do this using autoconf checks.
> 
> The issue is that this will break scons builds unless these checks are
> replicated there.  And SCons implementation of configure checks are not
> great to be honest -- they either are cached (but in such way were multiple
> builds from same source tree pick up wrong values) or they need to be
> re-checked on every build (wasting time for incremental builds).
> 
> This is why, within reason, I personally like to avoid configure checks when
> practical.
> 
> 
> So for now I'd prefer to leave MinGW 'fixed' as you put it.
> 
> But fell free to fix the other platforms as you propose.

If it isn't going to be configure checks could someone merge the
original patch in this thread?
On Wed, Feb 25, 2015 at 5:37 PM, Jonathan Gray <jsg@jsg.id.au> wrote:
> If it isn't going to be configure checks could someone merge the
> original patch in this thread?

I committed

commit 3492e88090d2d0c0bfbc934963b8772b45fc8880
Author: Matt Turner <mattst88@gmail.com>
Date:   Fri Feb 20 18:46:43 2015 -0800

    gallium/util: Use HAVE___BUILTIN_* macros.

    Reviewed-by: Eric Anholt <eric@anholt.net>
    Reviewed-by: Jose Fonseca <jfonseca@vmware.com>

which switched over a bunch of preprocessor checks around __builtin*
calls to use the macros defined by autotools.

So I think cleaning it up to use __builtin_ffs* first #ifdef
HAVE___BUILTIN_* can go forward now.
On Wed, Feb 25, 2015 at 06:53:14PM -0800, Matt Turner wrote:
> On Wed, Feb 25, 2015 at 5:37 PM, Jonathan Gray <jsg@jsg.id.au> wrote:
> > If it isn't going to be configure checks could someone merge the
> > original patch in this thread?
> 
> I committed
> 
> commit 3492e88090d2d0c0bfbc934963b8772b45fc8880
> Author: Matt Turner <mattst88@gmail.com>
> Date:   Fri Feb 20 18:46:43 2015 -0800
> 
>     gallium/util: Use HAVE___BUILTIN_* macros.
> 
>     Reviewed-by: Eric Anholt <eric@anholt.net>
>     Reviewed-by: Jose Fonseca <jfonseca@vmware.com>
> 
> which switched over a bunch of preprocessor checks around __builtin*
> calls to use the macros defined by autotools.
> 
> So I think cleaning it up to use __builtin_ffs* first #ifdef
> HAVE___BUILTIN_* can go forward now.

Yes but there is no HAVE_FFSLL for constructs like

#if !defined(HAVE_FFSLL) && defined(HAVE___BUILTIN_FFSLL)

or is it ok to always use the builtin?
On Wed, Feb 25, 2015 at 7:03 PM, Jonathan Gray <jsg@jsg.id.au> wrote:
> On Wed, Feb 25, 2015 at 06:53:14PM -0800, Matt Turner wrote:
>> On Wed, Feb 25, 2015 at 5:37 PM, Jonathan Gray <jsg@jsg.id.au> wrote:
>> > If it isn't going to be configure checks could someone merge the
>> > original patch in this thread?
>>
>> I committed
>>
>> commit 3492e88090d2d0c0bfbc934963b8772b45fc8880
>> Author: Matt Turner <mattst88@gmail.com>
>> Date:   Fri Feb 20 18:46:43 2015 -0800
>>
>>     gallium/util: Use HAVE___BUILTIN_* macros.
>>
>>     Reviewed-by: Eric Anholt <eric@anholt.net>
>>     Reviewed-by: Jose Fonseca <jfonseca@vmware.com>
>>
>> which switched over a bunch of preprocessor checks around __builtin*
>> calls to use the macros defined by autotools.
>>
>> So I think cleaning it up to use __builtin_ffs* first #ifdef
>> HAVE___BUILTIN_* can go forward now.
>
> Yes but there is no HAVE_FFSLL for constructs like
>
> #if !defined(HAVE_FFSLL) && defined(HAVE___BUILTIN_FFSLL)
>
> or is it ok to always use the builtin?

I think the question is whether it's okay to always use the builtin if
it's available (as opposed to libc functions). I think the answer to
that is yes.