Patchwork [v2,01/12] host-utils: add ffsl

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 16, 2013, 5:31 p.m.
Message ID <1358357479-7912-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/212859/
State New
Headers show

Comments

Paolo Bonzini - Jan. 16, 2013, 5:31 p.m.
We can provide fast versions based on the other functions defined
by host-utils.h.  Some care is required on glibc, which provides
ffsl already.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/host-utils.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Peter Maydell - Jan. 30, 2013, 2:05 p.m.
On 16 January 2013 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
> We can provide fast versions based on the other functions defined
> by host-utils.h.  Some care is required on glibc, which provides
> ffsl already.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Hi. This patch breaks compile on MacOS (and likely other BSDs):

  CC    qga/commands-posix.o
In file included from qga/commands-posix.c:22:
/Users/pm215/src/qemu/include/qemu/host-utils.h:248: error: static
declaration of ‘ffsl’ follows non-static declaration

(because BSD strings.h provides a system ffsl()).

> +/* glibc does not provide an inline version of ffsl, so always define
> + * ours.  We need to give it a different name, however.
> + */
> +#ifdef __GLIBC__
> +#define ffsl qemu_ffsl
> +#endif

Overriding a system function like this seems a bit brittle --
can't we either (a) always use the system version or (b)
always use a QEMU implementation?

Failing that, you probably need a configure test to check for
system ffsl().

thanks
-- PMM
Peter Maydell - Jan. 30, 2013, 3:03 p.m.
On 30 January 2013 14:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 January 2013 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> We can provide fast versions based on the other functions defined
>> by host-utils.h.  Some care is required on glibc, which provides
>> ffsl already.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Hi. This patch breaks compile on MacOS (and likely other BSDs):
>
>   CC    qga/commands-posix.o
> In file included from qga/commands-posix.c:22:
> /Users/pm215/src/qemu/include/qemu/host-utils.h:248: error: static
> declaration of ‘ffsl’ follows non-static declaration
>
> (because BSD strings.h provides a system ffsl()).
>
>> +/* glibc does not provide an inline version of ffsl, so always define
>> + * ours.  We need to give it a different name, however.
>> + */
>> +#ifdef __GLIBC__
>> +#define ffsl qemu_ffsl
>> +#endif
>
> Overriding a system function like this seems a bit brittle --
> can't we either (a) always use the system version or (b)
> always use a QEMU implementation?
>
> Failing that, you probably need a configure test to check for
> system ffsl().

Also, we already have a roll-your-own ffsl, it's bitops_ffsl()
in bitops.h. Can we improve and use that implementation instead
of having a second one?

-- PMM
Stefan Hajnoczi - Jan. 31, 2013, 12:27 p.m.
On Wed, Jan 16, 2013 at 06:31:08PM +0100, Paolo Bonzini wrote:
> We can provide fast versions based on the other functions defined
> by host-utils.h.  Some care is required on glibc, which provides
> ffsl already.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/host-utils.h |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)

Speaking of ffsl() issues, mingw32 now complains about ffsl() being
undefined in hbitmap:

In file included from /home/stefanha/qemu/include/block/block_int.h:35:0,
                 from qemu-img.c:32:
/home/stefanha/qemu/include/qemu/hbitmap.h: In function 'hbitmap_iter_next':
/home/stefanha/qemu/include/qemu/hbitmap.h:173:5: warning: implicit declaration of function 'ffsl' [-Wimplicit-function-declaration]

Using mingw32-gcc-4.7.2-7.fc18.  I haven't looked into this but I
thought ffsl() was a gcc built-in and it wouldn't require any headers.

Stefan

Patch

diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
index 81c9a75..2a32be4 100644
--- a/include/qemu/host-utils.h
+++ b/include/qemu/host-utils.h
@@ -26,6 +26,7 @@ 
 #define HOST_UTILS_H 1
 
 #include "qemu/compiler.h"   /* QEMU_GNUC_PREREQ */
+#include <string.h>     /* ffsl */
 
 #if defined(__x86_64__)
 #define __HAVE_FAST_MULU64__
@@ -237,4 +238,29 @@  static inline int ctpop64(uint64_t val)
 #endif
 }
 
+/* glibc does not provide an inline version of ffsl, so always define
+ * ours.  We need to give it a different name, however.
+ */
+#ifdef __GLIBC__
+#define ffsl qemu_ffsl
+#endif
+static inline int ffsl(long val)
+{
+    if (!val) {
+        return 0;
+    }
+
+#if QEMU_GNUC_PREREQ(3, 4)
+    return __builtin_ctzl(val) + 1;
+#else
+    if (sizeof(long) == 4) {
+        return ctz32(val) + 1;
+    } else if (sizeof(long) == 8) {
+        return ctz64(val) + 1;
+    } else {
+        abort();
+    }
+#endif
+}
+
 #endif