[Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Paolo Bonzini]

Three fixes: 1) Darwin does not support weak aliases, use weak
references instead.  2) Darwin, NetBSD and OpenBSD do not have
sem_timedwait, implement counting semaphores with a mutex and
cv there.  3) Daemonize was broken, fixes are in patches 3-5.

Paolo Bonzini (5):
  compiler: support Darwin weak references
  semaphore: implement fallback counting semaphores with mutex+condvar
  qemu-timer: reinitialize timers after fork
  vl: unify calls to init_timer_alarm
  vl: delay thread initialization after daemonization

 compiler.h          |  9 +++++-
 main-loop.c         |  6 ++--
 osdep.c             | 56 ++++++++++++++++++--------------
 oslib-win32.c       | 12 ++++---
 qemu-sockets.c      | 40 ++++++++++++-----------
 qemu-thread-posix.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++------
 qemu-thread-posix.h |  6 ++++
 qemu-timer.c        | 14 ++++++++
 qmp.c               |  2 ++
 vl.c                |  9 ++----
 10 file modificati, 180 inserzioni(+), 66 rimozioni(-)

-- 
1.7.12.1

[Qemu-devel] [PATCH v2 1/5] compiler: support Darwin weak references

[Paolo Bonzini]

Weakrefs only tell you if the symbol was defined elsewhere, so you
need a further check at runtime to pick the default definition
when needed.

This could be automated by the compiler, but it does not do it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: add unused attribute

 compiler.h     |  9 ++++++++-
 osdep.c        | 56 ++++++++++++++++++++++++++++++++------------------------
 oslib-win32.c  | 12 +++++++-----
 qemu-sockets.c | 40 ++++++++++++++++++++++------------------
 qmp.c          |  2 ++
 5 file modificati, 71 inserzioni(+), 48 rimozioni(-)

diff --git a/compiler.h b/compiler.h
index 58865d6..55d7d74 100644
--- a/compiler.h
+++ b/compiler.h
@@ -50,8 +50,15 @@
 #   define __printf__ __gnu_printf__
 #  endif
 # endif
-# define QEMU_WEAK_ALIAS(newname, oldname) \
+# if defined(__APPLE__)
+#  define QEMU_WEAK_ALIAS(newname, oldname) \
+        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#oldname)))
+#  define QEMU_WEAK_REF(newname, oldname) (weak_##newname ? weak_##newname : oldname)
+# else
+#  define QEMU_WEAK_ALIAS(newname, oldname) \
         typeof(oldname) newname __attribute__((weak, alias (#oldname)))
+#  define QEMU_WEAK_REF(newname, oldname) newname
+# endif
 #else
 #define GCC_ATTR /**/
 #define GCC_FMT_ATTR(n, m)
diff --git a/osdep.c b/osdep.c
index a87d4a4..2f7a491 100644
--- a/osdep.c
+++ b/osdep.c
@@ -54,6 +54,38 @@ static bool fips_enabled = false;
 
 static const char *qemu_version = QEMU_VERSION;
 
+static int default_fdset_get_fd(int64_t fdset_id, int flags)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
+#define monitor_fdset_get_fd \
+    QEMU_WEAK_REF(monitor_fdset_get_fd, default_fdset_get_fd)
+
+static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
+#define monitor_fdset_dup_fd_add \
+    QEMU_WEAK_REF(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add)
+
+static int default_fdset_dup_fd_remove(int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
+#define monitor_fdset_dup_fd_remove \
+    QEMU_WEAK_REF(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove)
+
+static int default_fdset_dup_fd_find(int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
+#define monitor_fdset_dup_fd_find \
+    QEMU_WEAK_REF(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_find)
+
 int socket_set_cork(int fd, int v)
 {
 #if defined(SOL_TCP) && defined(TCP_CORK)
@@ -400,27 +432,3 @@ bool fips_get_state(void)
     return fips_enabled;
 }
 
-
-static int default_fdset_get_fd(int64_t fdset_id, int flags)
-{
-    return -1;
-}
-QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
-
-static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
-{
-    return -1;
-}
-QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
-
-static int default_fdset_dup_fd_remove(int dup_fd)
-{
-    return -1;
-}
-QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
-
-static int default_fdset_dup_fd_find(int dup_fd)
-{
-    return -1;
-}
-QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
diff --git a/oslib-win32.c b/oslib-win32.c
index 9ca83df..326a2bd 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -32,6 +32,13 @@
 #include "trace.h"
 #include "qemu_socket.h"
 
+static void default_qemu_fd_register(int fd)
+{
+}
+QEMU_WEAK_ALIAS(qemu_fd_register, default_qemu_fd_register);
+#define qemu_fd_register \
+    QEMU_WEAK_REF(qemu_fd_register, default_qemu_fd_register)
+
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
@@ -150,8 +157,3 @@ int qemu_get_thread_id(void)
 {
     return GetCurrentThreadId();
 }
-
-static void default_qemu_fd_register(int fd)
-{
-}
-QEMU_WEAK_ALIAS(qemu_fd_register, default_qemu_fd_register);
diff --git a/qemu-sockets.c b/qemu-sockets.c
index f2a6371..abcd791 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -61,6 +61,28 @@ static QemuOptsList dummy_opts = {
     },
 };
 
+static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
+{
+    error_setg(errp, "only QEMU supports file descriptor passing");
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_get_fd, default_monitor_get_fd);
+#define monitor_get_fd \
+    QEMU_WEAK_REF(monitor_get_fd, default_monitor_get_fd)
+
+static int default_qemu_set_fd_handler2(int fd,
+                                        IOCanReadHandler *fd_read_poll,
+                                        IOHandler *fd_read,
+                                        IOHandler *fd_write,
+                                        void *opaque)
+
+{
+    abort();
+}
+QEMU_WEAK_ALIAS(qemu_set_fd_handler2, default_qemu_set_fd_handler2);
+#define qemu_set_fd_handler2 \
+    QEMU_WEAK_REF(qemu_set_fd_handler2, default_qemu_set_fd_handler2)
+
 static int inet_getport(struct addrinfo *e)
 {
     struct sockaddr_in *i4;
@@ -967,21 +989,3 @@ int socket_init(void)
 #endif
     return 0;
 }
-
-static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
-{
-    error_setg(errp, "only QEMU supports file descriptor passing");
-    return -1;
-}
-QEMU_WEAK_ALIAS(monitor_get_fd, default_monitor_get_fd);
-
-static int default_qemu_set_fd_handler2(int fd,
-                                        IOCanReadHandler *fd_read_poll,
-                                        IOHandler *fd_read,
-                                        IOHandler *fd_write,
-                                        void *opaque)
-
-{
-    abort();
-}
-QEMU_WEAK_ALIAS(qemu_set_fd_handler2, default_qemu_set_fd_handler2);
diff --git a/qmp.c b/qmp.c
index 638888a..13e83a5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -477,6 +477,8 @@ static CpuDefinitionInfoList *default_arch_query_cpu_definitions(Error **errp)
     return NULL;
 }
 QEMU_WEAK_ALIAS(arch_query_cpu_definitions, default_arch_query_cpu_definitions);
+#define arch_query_cpu_definitions \
+    QEMU_WEAK_REF(arch_query_cpu_definitions, default_arch_query_cpu_definitions)
 
 CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
 {
-- 
1.7.12.1

[Qemu-devel] [PATCH v2 2/5] semaphore: implement fallback counting semaphores with mutex+condvar

[Paolo Bonzini]

OpenBSD and Darwin do not have sem_timedwait.  Implement a fallback
for them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: extract compute_abs_deadline and use it

 qemu-thread-posix.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++------
 qemu-thread-posix.h |  6 ++++
 2 file modificati, 88 inserzioni(+), 10 rimozioni(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index 6a3d3a1..4ef9c7b 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -122,36 +122,106 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
 {
     int rc;
 
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    rc = pthread_mutex_init(&sem->lock, NULL);
+    if (rc != 0) {
+        error_exit(rc, __func__);
+    }
+    rc = pthread_cond_init(&sem->cond, NULL);
+    if (rc != 0) {
+        error_exit(rc, __func__);
+    }
+    if (init < 0) {
+        error_exit(EINVAL, __func__);
+    }
+    sem->count = init;
+#else
     rc = sem_init(&sem->sem, 0, init);
     if (rc < 0) {
         error_exit(errno, __func__);
     }
+#endif
 }
 
 void qemu_sem_destroy(QemuSemaphore *sem)
 {
     int rc;
 
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    rc = pthread_cond_destroy(&sem->cond);
+    if (rc < 0) {
+        error_exit(rc, __func__);
+    }
+    rc = pthread_mutex_destroy(&sem->lock);
+    if (rc < 0) {
+        error_exit(rc, __func__);
+    }
+#else
     rc = sem_destroy(&sem->sem);
     if (rc < 0) {
         error_exit(errno, __func__);
     }
+#endif
 }
 
 void qemu_sem_post(QemuSemaphore *sem)
 {
     int rc;
 
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    pthread_mutex_lock(&sem->lock);
+    if (sem->count == INT_MAX) {
+        rc = EINVAL;
+    } else if (sem->count++ < 0) {
+        rc = pthread_cond_signal(&sem->cond);
+    } else {
+        rc = 0;
+    }
+    pthread_mutex_unlock(&sem->lock);
+    if (rc != 0) {
+        error_exit(rc, __func__);
+    }
+#else
     rc = sem_post(&sem->sem);
     if (rc < 0) {
         error_exit(errno, __func__);
     }
+#endif
+}
+
+static void compute_abs_deadline(struct timespec *ts, int ms)
+{
+    struct timeval tv;
+    gettimeofday(&tv, NULL);
+    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
+    ts->tv_sec = tv.tv_sec + ms / 1000;
+    if (ts->tv_nsec >= 1000000000) {
+        ts->tv_sec++;
+        ts->tv_nsec -= 1000000000;
+    }
 }
 
 int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
 {
     int rc;
-
+    struct timespec ts;
+
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    compute_abs_deadline(&ts, ms);
+    pthread_mutex_lock(&sem->lock);
+    --sem->count;
+    while (sem->count < 0) {
+        rc = pthread_cond_timedwait(&sem->cond, &sem->lock, &ts);
+        if (rc == ETIMEDOUT) {
+            break;
+        }
+        if (rc != 0) {
+            error_exit(rc, __func__);
+        }
+    }
+    pthread_mutex_unlock(&sem->lock);
+    return (rc == ETIMEDOUT ? -1 : 0);
+#else
     if (ms <= 0) {
         /* This is cheaper than sem_timedwait.  */
         do {
@@ -161,15 +231,7 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
             return -1;
         }
     } else {
-        struct timeval tv;
-        struct timespec ts;
-        gettimeofday(&tv, NULL);
-        ts.tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
-        ts.tv_sec = tv.tv_sec + ms / 1000;
-        if (ts.tv_nsec >= 1000000000) {
-            ts.tv_sec++;
-            ts.tv_nsec -= 1000000000;
-        }
+        compute_abs_deadline(&ts, ms);
         do {
             rc = sem_timedwait(&sem->sem, &ts);
         } while (rc == -1 && errno == EINTR);
@@ -181,10 +243,19 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
         error_exit(errno, __func__);
     }
     return 0;
+#endif
 }
 
 void qemu_sem_wait(QemuSemaphore *sem)
 {
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    pthread_mutex_lock(&sem->lock);
+    --sem->count;
+    while (sem->count < 0) {
+        pthread_cond_wait(&sem->cond, &sem->lock);
+    }
+    pthread_mutex_unlock(&sem->lock);
+#else
     int rc;
 
     do {
@@ -193,6 +264,7 @@ void qemu_sem_wait(QemuSemaphore *sem)
     if (rc < 0) {
         error_exit(errno, __func__);
     }
+#endif
 }
 
 void qemu_thread_create(QemuThread *thread,
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index 2542c15..380bae2 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -12,7 +12,13 @@ struct QemuCond {
 };
 
 struct QemuSemaphore {
+#if defined(__OpenBSD__) || defined(__APPLE__) || defined(__NetBSD__)
+    pthread_mutex_t lock;
+    pthread_cond_t cond;
+    int count;
+#else
     sem_t sem;
+#endif
 };
 
 struct QemuThread {
-- 
1.7.12.1

[Qemu-devel] [PATCH v2 3/5] qemu-timer: reinitialize timers after fork

[Paolo Bonzini]

Timers are not inherited by the child of a fork(2), so just use
pthread_atfork to reinstate them after daemonize.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c | 14 ++++++++++++++
 1 file modificato, 14 inserzioni(+)

diff --git a/qemu-timer.c b/qemu-timer.c
index f3426c9..7b2217a 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -742,6 +742,17 @@ static void quit_timers(void)
     t->stop(t);
 }
 
+static void reinit_timers(void)
+{
+    struct qemu_alarm_timer *t = alarm_timer;
+    t->stop(t);
+    if (t->start(t)) {
+        fprintf(stderr, "Internal timer error: aborting\n");
+        exit(1);
+    }
+    qemu_rearm_alarm_timer(t);
+}
+
 int init_timer_alarm(void)
 {
     struct qemu_alarm_timer *t = NULL;
@@ -765,6 +776,9 @@ int init_timer_alarm(void)
     }
 
     atexit(quit_timers);
+#ifdef CONFIG_POSIX
+    pthread_atfork(NULL, NULL, reinit_timers);
+#endif
     alarm_timer = t;
     return 0;
 
-- 
1.7.12.1

[Qemu-devel] [PATCH v2 4/5] vl: unify calls to init_timer_alarm

[Paolo Bonzini]

init_timer_alarm was being called twice.  This is not needed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c | 5 ++++-
 vl.c        | 5 -----
 2 file modificati, 4 inserzioni(+), 6 rimozioni(-)

diff --git a/main-loop.c b/main-loop.c
index e43c7c8..234a313 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -123,7 +123,10 @@ int qemu_init_main_loop(void)
     GSource *src;
 
     init_clocks();
-    init_timer_alarm();
+    if (init_timer_alarm() < 0) {
+        fprintf(stderr, "could not initialize alarm timer\n");
+        exit(1);
+    }
 
     qemu_mutex_lock_iothread();
     ret = qemu_signal_init();
diff --git a/vl.c b/vl.c
index 99681da..e2d5276 100644
--- a/vl.c
+++ b/vl.c
@@ -3616,11 +3616,6 @@ int main(int argc, char **argv, char **envp)
             add_device_config(DEV_VIRTCON, "vc:80Cx24C");
     }
 
-    if (init_timer_alarm() < 0) {
-        fprintf(stderr, "could not initialize alarm timer\n");
-        exit(1);
-    }
-
     socket_init();
 
     if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
-- 
1.7.12.1

[Qemu-devel] [PATCH v2 5/5] vl: delay thread initialization after daemonization

[Paolo Bonzini]

Commit ac4119c (chardev: Use timer instead of bottom-half to postpone
open event, 2012-10-12) moved the alarm timer initialization to an earlier
point but failed to consider that it depends on qemu_init_main_loop.

Later, commit 1c53786 (vl: init main loop earlier, 2012-10-30) fixed
this, but left -daemonize in two different ways.  First, timers need to
be reinitialized after forking.  Second, the global mutex was being held
by the parent, and thus dropped after forking.

The first is now fixed using pthread_atfork.  For the second part,
make sure that the global mutex is not taken before daemonization,
and similarly delay qemu_thread_self.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c | 1 -
 vl.c        | 4 +++-
 2 file modificati, 3 inserzioni(+), 2 rimozioni(-)

diff --git a/main-loop.c b/main-loop.c
index 234a313..c87624e 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -128,7 +128,6 @@ int qemu_init_main_loop(void)
         exit(1);
     }
 
-    qemu_mutex_lock_iothread();
     ret = qemu_signal_init();
     if (ret) {
         return ret;
diff --git a/vl.c b/vl.c
index e2d5276..0f5b07b 100644
--- a/vl.c
+++ b/vl.c
@@ -3477,7 +3477,6 @@ int main(int argc, char **argv, char **envp)
     }
     loc_set_none();
 
-    qemu_init_cpu_loop();
     if (qemu_init_main_loop()) {
         fprintf(stderr, "qemu_init_main_loop failed\n");
         exit(1);
@@ -3677,6 +3676,9 @@ int main(int argc, char **argv, char **envp)
 
     os_set_line_buffering();
 
+    qemu_init_cpu_loop();
+    qemu_mutex_lock_iothread();
+
 #ifdef CONFIG_SPICE
     /* spice needs the timers to be initialized by this point */
     qemu_spice_init();
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Peter Maydell]

On 2 November 2012 15:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Three fixes: 1) Darwin does not support weak aliases, use weak
> references instead.  2) Darwin, NetBSD and OpenBSD do not have
> sem_timedwait, implement counting semaphores with a mutex and
> cv there.  3) Daemonize was broken, fixes are in patches 3-5.

v2 patches 1 & 2 compile cleanly on macos and the resulting
qemu seems to work (smoke tested only).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Blue Swirl]

On Fri, Nov 2, 2012 at 3:12 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 November 2012 15:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Three fixes: 1) Darwin does not support weak aliases, use weak
>> references instead.  2) Darwin, NetBSD and OpenBSD do not have
>> sem_timedwait, implement counting semaphores with a mutex and
>> cv there.  3) Daemonize was broken, fixes are in patches 3-5.
>
> v2 patches 1 & 2 compile cleanly on macos and the resulting
> qemu seems to work (smoke tested only).

I'm still getting problems with Clang on Linux:

  CC    qemu-sockets.o
/src/qemu/qemu-sockets.c:64:12: error: function
'default_monitor_get_fd' is not needed and will not be emitted
[-Werror,-Wunneeded-internal-declaration]
static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
           ^
/src/qemu/qemu-sockets.c:73:12: error: function
'default_qemu_set_fd_handler2' is not needed and will not be emitted
[-Werror,-Wunneeded-internal-declaration]
static int default_qemu_set_fd_handler2(int fd,
           ^
2 errors generated.

Perhaps the weak magic isn't so great after all.

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Paolo Bonzini]

Il 03/11/2012 12:50, Blue Swirl ha scritto:
> I'm still getting problems with Clang on Linux:
> 
>   CC    qemu-sockets.o
> /src/qemu/qemu-sockets.c:64:12: error: function
> 'default_monitor_get_fd' is not needed and will not be emitted
> [-Werror,-Wunneeded-internal-declaration]
> static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>            ^
> /src/qemu/qemu-sockets.c:73:12: error: function
> 'default_qemu_set_fd_handler2' is not needed and will not be emitted
> [-Werror,-Wunneeded-internal-declaration]
> static int default_qemu_set_fd_handler2(int fd,
>            ^
> 2 errors generated.
> 
> Perhaps the weak magic isn't so great after all.
> 

It's a clang bug.  The error should be suppressed, since the function is
used with the weak alias.

Or try if adding "|| defined __clang__" to compiler.h fixes it.

Paolo

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Paolo Bonzini]

Il 03/11/2012 15:19, Paolo Bonzini ha scritto:
> Il 03/11/2012 12:50, Blue Swirl ha scritto:
>> I'm still getting problems with Clang on Linux:
>>
>>   CC    qemu-sockets.o
>> /src/qemu/qemu-sockets.c:64:12: error: function
>> 'default_monitor_get_fd' is not needed and will not be emitted
>> [-Werror,-Wunneeded-internal-declaration]
>> static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>>            ^
>> /src/qemu/qemu-sockets.c:73:12: error: function
>> 'default_qemu_set_fd_handler2' is not needed and will not be emitted
>> [-Werror,-Wunneeded-internal-declaration]
>> static int default_qemu_set_fd_handler2(int fd,
>>            ^
>> 2 errors generated.
>>
>> Perhaps the weak magic isn't so great after all.
>>
> 
> It's a clang bug.  The error should be suppressed, since the function is
> used with the weak alias.
> 
> Or try if adding "|| defined __clang__" to compiler.h fixes it.

Other possible fixes include adding "inline" to the declarations, or of
course -Wno-unneeded-internal-declaration.

Paolo

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Blue Swirl]

On Sat, Nov 3, 2012 at 2:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 03/11/2012 12:50, Blue Swirl ha scritto:
>> I'm still getting problems with Clang on Linux:
>>
>>   CC    qemu-sockets.o
>> /src/qemu/qemu-sockets.c:64:12: error: function
>> 'default_monitor_get_fd' is not needed and will not be emitted
>> [-Werror,-Wunneeded-internal-declaration]
>> static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>>            ^
>> /src/qemu/qemu-sockets.c:73:12: error: function
>> 'default_qemu_set_fd_handler2' is not needed and will not be emitted
>> [-Werror,-Wunneeded-internal-declaration]
>> static int default_qemu_set_fd_handler2(int fd,
>>            ^
>> 2 errors generated.
>>
>> Perhaps the weak magic isn't so great after all.
>>
>
> It's a clang bug.  The error should be suppressed, since the function is
> used with the weak alias.
>
> Or try if adding "|| defined __clang__" to compiler.h fixes it.

It does. Maybe __APPLE__ should be replaced by __clang__, or can the
problem also occur with GCC on Apple systems?

>
> Paolo

Re: [Qemu-devel] [PATCH v2 0/5] Fixes for thread pool patches.

[Paolo Bonzini]

Il 03/11/2012 16:26, Blue Swirl ha scritto:
>> > It's a clang bug.  The error should be suppressed, since the function is
>> > used with the weak alias.
>> >
>> > Or try if adding "|| defined __clang__" to compiler.h fixes it.
> It does.

Yeah, I tested now Clang/Linux myself and reported it as
http://llvm.org/bugs/show_bug.cgi?id=14250.  With a simple testcase such
as this:

static int g()
{
    return 42;
}
typeof(g) f __attribute__((__weak__, __alias__("g")));


it's easy to see that the function is actually emitted---and with "g" as
the name, even:

    .file    "g2.c"
    .text
    .align    16, 0x90
    .type    g,@function
g:                                      # @g
    .cfi_startproc
# BB#0:
    movl    $42, %eax
    ret
.Ltmp0:
    .size    g, .Ltmp0-g
    .cfi_endproc

    .weak    f
f = g
    .section    ".note.GNU-stack","",@progbits

> Maybe __APPLE__ should be replaced by __clang__, or can the
> problem also occur with GCC on Apple systems?

It can occur with GCC, though it will be just a "note" if I understand
correctly and not fail with -Werror.  No idea whether the generated code
would run, probably not.

My favorite fix is to just disable the warning, otherwise make the
function inline.

Paolo

Re: [Qemu-devel] [PATCH v2 1/5] compiler: support Darwin weak references

[TeLeMan]

On Fri, Nov 2, 2012 at 10:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Weakrefs only tell you if the symbol was defined elsewhere, so you
> need a further check at runtime to pick the default definition
> when needed.
>
> This could be automated by the compiler, but it does not do it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         v1->v2: add unused attribute
>
>  compiler.h     |  9 ++++++++-
>  osdep.c        | 56 ++++++++++++++++++++++++++++++++------------------------
>  oslib-win32.c  | 12 +++++++-----
>  qemu-sockets.c | 40 ++++++++++++++++++++++------------------
>  qmp.c          |  2 ++
>  5 file modificati, 71 inserzioni(+), 48 rimozioni(-)
>
> diff --git a/compiler.h b/compiler.h
> index 58865d6..55d7d74 100644
> --- a/compiler.h
> +++ b/compiler.h
> @@ -50,8 +50,15 @@
>  #   define __printf__ __gnu_printf__
>  #  endif
>  # endif
> -# define QEMU_WEAK_ALIAS(newname, oldname) \
> +# if defined(__APPLE__)
> +#  define QEMU_WEAK_ALIAS(newname, oldname) \
> +        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#oldname)))
> +#  define QEMU_WEAK_REF(newname, oldname) (weak_##newname ? weak_##newname : oldname)
> +# else
> +#  define QEMU_WEAK_ALIAS(newname, oldname) \
>          typeof(oldname) newname __attribute__((weak, alias (#oldname)))
> +#  define QEMU_WEAK_REF(newname, oldname) newname
> +# endif
>  #else
>  #define GCC_ATTR /**/
>  #define GCC_FMT_ATTR(n, m)
> diff --git a/osdep.c b/osdep.c
> index a87d4a4..2f7a491 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -54,6 +54,38 @@ static bool fips_enabled = false;
>
>  static const char *qemu_version = QEMU_VERSION;
>
> +static int default_fdset_get_fd(int64_t fdset_id, int flags)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
> +#define monitor_fdset_get_fd \
> +    QEMU_WEAK_REF(monitor_fdset_get_fd, default_fdset_get_fd)
> +
> +static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
> +#define monitor_fdset_dup_fd_add \
> +    QEMU_WEAK_REF(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add)
> +
> +static int default_fdset_dup_fd_remove(int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
> +#define monitor_fdset_dup_fd_remove \
> +    QEMU_WEAK_REF(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove)
> +
> +static int default_fdset_dup_fd_find(int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
> +#define monitor_fdset_dup_fd_find \
> +    QEMU_WEAK_REF(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_find)
Should here be QEMU_WEAK_REF(monitor_fdset_dup_fd_find,
default_fdset_dup_fd_find)?

Another issue: Some weird codes were generated on MinGW + gcc 4.5.1:

244         fdset_id = monitor_fdset_dup_fd_find(fd);
(gdb) x/3i $pc
=> 0x553e9e <qemu_close+22>:    mov    -0x1c(%ebp),%eax
   0x553ea1 <qemu_close+25>:    mov    %eax,(%esp)
   0x553ea4 <qemu_close+28>:
    call   0x637bb6 <monitor_fdset_dup_fd_find_remove+29>

It's called directly into the middle of monitor_fdset_dup_fd_find_remove.

> +
>  int socket_set_cork(int fd, int v)
>  {
>  #if defined(SOL_TCP) && defined(TCP_CORK)
> @@ -400,27 +432,3 @@ bool fips_get_state(void)
>      return fips_enabled;
>  }
>
> -
> -static int default_fdset_get_fd(int64_t fdset_id, int flags)
> -{
> -    return -1;
> -}
> -QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
> -
> -static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> -{
> -    return -1;
> -}
> -QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
> -
> -static int default_fdset_dup_fd_remove(int dup_fd)
> -{
> -    return -1;
> -}
> -QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
> -
> -static int default_fdset_dup_fd_find(int dup_fd)
> -{
> -    return -1;
> -}
> -QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
> diff --git a/oslib-win32.c b/oslib-win32.c
> index 9ca83df..326a2bd 100644
> --- a/oslib-win32.c
> +++ b/oslib-win32.c
> @@ -32,6 +32,13 @@
>  #include "trace.h"
>  #include "qemu_socket.h"
>
> +static void default_qemu_fd_register(int fd)
> +{
> +}
> +QEMU_WEAK_ALIAS(qemu_fd_register, default_qemu_fd_register);
> +#define qemu_fd_register \
> +    QEMU_WEAK_REF(qemu_fd_register, default_qemu_fd_register)
> +
>  void *qemu_oom_check(void *ptr)
>  {
>      if (ptr == NULL) {
> @@ -150,8 +157,3 @@ int qemu_get_thread_id(void)
>  {
>      return GetCurrentThreadId();
>  }
> -
> -static void default_qemu_fd_register(int fd)
> -{
> -}
> -QEMU_WEAK_ALIAS(qemu_fd_register, default_qemu_fd_register);
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index f2a6371..abcd791 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -61,6 +61,28 @@ static QemuOptsList dummy_opts = {
>      },
>  };
>
> +static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
> +{
> +    error_setg(errp, "only QEMU supports file descriptor passing");
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_get_fd, default_monitor_get_fd);
> +#define monitor_get_fd \
> +    QEMU_WEAK_REF(monitor_get_fd, default_monitor_get_fd)
> +
> +static int default_qemu_set_fd_handler2(int fd,
> +                                        IOCanReadHandler *fd_read_poll,
> +                                        IOHandler *fd_read,
> +                                        IOHandler *fd_write,
> +                                        void *opaque)
> +
> +{
> +    abort();
> +}
> +QEMU_WEAK_ALIAS(qemu_set_fd_handler2, default_qemu_set_fd_handler2);
> +#define qemu_set_fd_handler2 \
> +    QEMU_WEAK_REF(qemu_set_fd_handler2, default_qemu_set_fd_handler2)
> +
>  static int inet_getport(struct addrinfo *e)
>  {
>      struct sockaddr_in *i4;
> @@ -967,21 +989,3 @@ int socket_init(void)
>  #endif
>      return 0;
>  }
> -
> -static int default_monitor_get_fd(Monitor *mon, const char *name, Error **errp)
> -{
> -    error_setg(errp, "only QEMU supports file descriptor passing");
> -    return -1;
> -}
> -QEMU_WEAK_ALIAS(monitor_get_fd, default_monitor_get_fd);
> -
> -static int default_qemu_set_fd_handler2(int fd,
> -                                        IOCanReadHandler *fd_read_poll,
> -                                        IOHandler *fd_read,
> -                                        IOHandler *fd_write,
> -                                        void *opaque)
> -
> -{
> -    abort();
> -}
> -QEMU_WEAK_ALIAS(qemu_set_fd_handler2, default_qemu_set_fd_handler2);
> diff --git a/qmp.c b/qmp.c
> index 638888a..13e83a5 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -477,6 +477,8 @@ static CpuDefinitionInfoList *default_arch_query_cpu_definitions(Error **errp)
>      return NULL;
>  }
>  QEMU_WEAK_ALIAS(arch_query_cpu_definitions, default_arch_query_cpu_definitions);
> +#define arch_query_cpu_definitions \
> +    QEMU_WEAK_REF(arch_query_cpu_definitions, default_arch_query_cpu_definitions)
>
>  CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>  {
> --
> 1.7.12.1
>
>
>