[PATCH 0/3] TSA: make sure QEMU compiles when using clang TSA

[PATCH 0/3] TSA: make sure QEMU compiles when using clang TSA

[Emanuele Giuseppe Esposito]

This serie aims to enable clang Thread Safety Analysis (TSA) in QEMU.
The goal is to use it for our multiqueue project aiming to replace the
block layer AioContext lock with a rwlock and make sure the lock is taken
correctly everywhere [1].

By default, TSA covers pthread mutexes, therefore when added in QEMU it
immediately detects some wrappers using pthread_mutex_lock/unlock without
using the proper TSA macros. Since adding such macro requires scanning all
possible callers of the affected wrapper, simply use TSA_NO_TSA to suppress
the warnings.

[1] = https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg00903.html

Emanuele Giuseppe Esposito (2):
  util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings
  bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

Kevin Wolf (1):
  configure: Enable -Wthread-safety if present

 configure                |  1 +
 bsd-user/qemu.h          |  5 +++--
 include/exec/exec-all.h  |  5 +++--
 include/qemu/thread.h    | 14 +++++++++-----
 util/qemu-thread-posix.c |  2 +-
 5 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.39.0

[PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings

[Emanuele Giuseppe Esposito]

QEMU does not compile when enabling clang's thread safety analysis
(TSA),
because some functions create wrappers for pthread mutexes but do
not use any TSA macro. Therefore the compiler fails.

In order to make the compiler happy and avoid adding all the
necessary macros to all callers (lock functions should use
TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
users of pthread_mutex_lock/pthread_mutex_unlock),
simply use TSA_NO_TSA to supppress such warnings.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/thread.h    | 14 +++++++++-----
 util/qemu-thread-posix.c |  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 7c6703bce3..81ec9fc144 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -3,6 +3,7 @@
 
 #include "qemu/processor.h"
 #include "qemu/atomic.h"
+#include "qemu/clang-tsa.h"
 
 typedef struct QemuCond QemuCond;
 typedef struct QemuSemaphore QemuSemaphore;
@@ -24,9 +25,12 @@ typedef struct QemuThread QemuThread;
 
 void qemu_mutex_init(QemuMutex *mutex);
 void qemu_mutex_destroy(QemuMutex *mutex);
-int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line);
-void qemu_mutex_lock_impl(QemuMutex *mutex, const char *file, const int line);
-void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int line);
+int TSA_NO_TSA qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file,
+                                       const int line);
+void TSA_NO_TSA qemu_mutex_lock_impl(QemuMutex *mutex, const char *file,
+                                     const int line);
+void TSA_NO_TSA qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file,
+                                       const int line);
 
 void qemu_rec_mutex_init(QemuRecMutex *mutex);
 void qemu_rec_mutex_destroy(QemuRecMutex *mutex);
@@ -153,8 +157,8 @@ void qemu_cond_destroy(QemuCond *cond);
  */
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
-void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex,
-                         const char *file, const int line);
+void TSA_NO_TSA qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex,
+                                    const char *file, const int line);
 bool qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
                               const char *file, const int line);
 
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index bae938c670..2dd1069cd3 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -223,7 +223,7 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
         error_exit(err, __func__);
 }
 
-static bool
+static bool TSA_NO_TSA
 qemu_cond_timedwait_ts(QemuCond *cond, QemuMutex *mutex, struct timespec *ts,
                        const char *file, const int line)
 {
-- 
2.39.0

Re: [PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings

[Philippe Mathieu-Daudé]

On 17/1/23 14:52, Emanuele Giuseppe Esposito wrote:
> QEMU does not compile when enabling clang's thread safety analysis
> (TSA),
> because some functions create wrappers for pthread mutexes but do
> not use any TSA macro. Therefore the compiler fails.
> 
> In order to make the compiler happy and avoid adding all the
> necessary macros to all callers (lock functions should use
> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
> users of pthread_mutex_lock/pthread_mutex_unlock),
> simply use TSA_NO_TSA to supppress such warnings.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/thread.h    | 14 +++++++++-----
>   util/qemu-thread-posix.c |  2 +-
>   2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index 7c6703bce3..81ec9fc144 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -3,6 +3,7 @@
>   
>   #include "qemu/processor.h"
>   #include "qemu/atomic.h"
> +#include "qemu/clang-tsa.h"

Missing file?     ^^^^^^^^^^

Re: [PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings

[Emanuele Giuseppe Esposito]

Am 17/01/2023 um 15:33 schrieb Philippe Mathieu-Daudé:
> On 17/1/23 14:52, Emanuele Giuseppe Esposito wrote:
>> QEMU does not compile when enabling clang's thread safety analysis
>> (TSA),
>> because some functions create wrappers for pthread mutexes but do
>> not use any TSA macro. Therefore the compiler fails.
>>
>> In order to make the compiler happy and avoid adding all the
>> necessary macros to all callers (lock functions should use
>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
>> users of pthread_mutex_lock/pthread_mutex_unlock),
>> simply use TSA_NO_TSA to supppress such warnings.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   include/qemu/thread.h    | 14 +++++++++-----
>>   util/qemu-thread-posix.c |  2 +-
>>   2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
>> index 7c6703bce3..81ec9fc144 100644
>> --- a/include/qemu/thread.h
>> +++ b/include/qemu/thread.h
>> @@ -3,6 +3,7 @@
>>     #include "qemu/processor.h"
>>   #include "qemu/atomic.h"
>> +#include "qemu/clang-tsa.h"
> 
> Missing file?     ^^^^^^^^^^
> 
? Forgot to pull latest changes? I see clang-tsa.h in master

Re: [PATCH 1/3] util/qemu-thread-posix: use TSA_NO_TSA to suppress clang TSA warnings

[Philippe Mathieu-Daudé]

On 17/1/23 15:43, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 17/01/2023 um 15:33 schrieb Philippe Mathieu-Daudé:
>> On 17/1/23 14:52, Emanuele Giuseppe Esposito wrote:
>>> QEMU does not compile when enabling clang's thread safety analysis
>>> (TSA),
>>> because some functions create wrappers for pthread mutexes but do
>>> not use any TSA macro. Therefore the compiler fails.
>>>
>>> In order to make the compiler happy and avoid adding all the
>>> necessary macros to all callers (lock functions should use
>>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
>>> users of pthread_mutex_lock/pthread_mutex_unlock),
>>> simply use TSA_NO_TSA to supppress such warnings.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>    include/qemu/thread.h    | 14 +++++++++-----
>>>    util/qemu-thread-posix.c |  2 +-
>>>    2 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
>>> index 7c6703bce3..81ec9fc144 100644
>>> --- a/include/qemu/thread.h
>>> +++ b/include/qemu/thread.h
>>> @@ -3,6 +3,7 @@
>>>      #include "qemu/processor.h"
>>>    #include "qemu/atomic.h"
>>> +#include "qemu/clang-tsa.h"
>>
>> Missing file?     ^^^^^^^^^^
>>
> ? Forgot to pull latest changes? I see clang-tsa.h in master

Oops sorry I forgot to reset a git-bisection, and indeed was
based on a older base :\

[PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Emanuele Giuseppe Esposito]

QEMU does not compile when enabling clang's thread safety analysis
(TSA),
because some functions create wrappers for pthread mutexes but do
not use any TSA macro. Therefore the compiler fails.

In order to make the compiler happy and avoid adding all the
necessary macros to all callers (lock functions should use
TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of pthread_mutex_lock/pthread_mutex_unlock),
simply use TSA_NO_TSA to supppress such warnings.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 bsd-user/qemu.h         | 5 +++--
 include/exec/exec-all.h | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index be6105385e..711fdd1b64 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -37,6 +37,7 @@ extern char **environ;
 #include "target_os_signal.h"
 #include "target.h"
 #include "exec/gdbstub.h"
+#include "qemu/clang-tsa.h"
 
 /*
  * This struct is used to hold certain information about the image.  Basically,
@@ -235,8 +236,8 @@ int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern unsigned long last_brk;
 extern abi_ulong mmap_next_start;
 abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size);
-void mmap_fork_start(void);
-void mmap_fork_end(int child);
+void TSA_NO_TSA mmap_fork_start(void);
+void TSA_NO_TSA mmap_fork_end(int child);
 
 /* main.c */
 extern char qemu_proc_pathname[];
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 25e11b0a8d..4f0c0559ac 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -25,6 +25,7 @@
 #include "exec/cpu_ldst.h"
 #endif
 #include "qemu/interval-tree.h"
+#include "qemu/clang-tsa.h"
 
 /* allow to see translation results - the slowdown should be negligible, so we leave it */
 #define DEBUG_DISAS
@@ -758,8 +759,8 @@ static inline tb_page_addr_t get_page_addr_code(CPUArchState *env,
 }
 
 #if defined(CONFIG_USER_ONLY)
-void mmap_lock(void);
-void mmap_unlock(void);
+void TSA_NO_TSA mmap_lock(void);
+void TSA_NO_TSA mmap_unlock(void);
 bool have_mmap_lock(void);
 
 /**
-- 
2.39.0

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2515 bytes --]

On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
eesposit@redhat.com> wrote:

> QEMU does not compile when enabling clang's thread safety analysis
> (TSA),
> because some functions create wrappers for pthread mutexes but do
> not use any TSA macro. Therefore the compiler fails.
>
> In order to make the compiler happy and avoid adding all the
> necessary macros to all callers (lock functions should use
> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> pthread_mutex_lock/pthread_mutex_unlock),
> simply use TSA_NO_TSA to supppress such warnings.
>

I'm not sure I understand this quite right. Maybe a clarifying question
will help me understand: Why is this needed for bsd-user but not
linux-user? How are they different here?

Warner


> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  bsd-user/qemu.h         | 5 +++--
>  include/exec/exec-all.h | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..711fdd1b64 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -37,6 +37,7 @@ extern char **environ;
>  #include "target_os_signal.h"
>  #include "target.h"
>  #include "exec/gdbstub.h"
> +#include "qemu/clang-tsa.h"
>
>  /*
>   * This struct is used to hold certain information about the image.
> Basically,
> @@ -235,8 +236,8 @@ int target_msync(abi_ulong start, abi_ulong len, int
> flags);
>  extern unsigned long last_brk;
>  extern abi_ulong mmap_next_start;
>  abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size);
> -void mmap_fork_start(void);
> -void mmap_fork_end(int child);
> +void TSA_NO_TSA mmap_fork_start(void);
> +void TSA_NO_TSA mmap_fork_end(int child);
>
>  /* main.c */
>  extern char qemu_proc_pathname[];
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 25e11b0a8d..4f0c0559ac 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -25,6 +25,7 @@
>  #include "exec/cpu_ldst.h"
>  #endif
>  #include "qemu/interval-tree.h"
> +#include "qemu/clang-tsa.h"
>
>  /* allow to see translation results - the slowdown should be negligible,
> so we leave it */
>  #define DEBUG_DISAS
> @@ -758,8 +759,8 @@ static inline tb_page_addr_t
> get_page_addr_code(CPUArchState *env,
>  }
>
>  #if defined(CONFIG_USER_ONLY)
> -void mmap_lock(void);
> -void mmap_unlock(void);
> +void TSA_NO_TSA mmap_lock(void);
> +void TSA_NO_TSA mmap_unlock(void);
>  bool have_mmap_lock(void);
>
>  /**
> --
> 2.39.0
>
>

[-- Attachment #2: Type: text/html, Size: 3370 bytes --]

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Emanuele Giuseppe Esposito]

Am 17/01/2023 um 17:16 schrieb Warner Losh:
> 
> 
> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito
> <eesposit@redhat.com <mailto:eesposit@redhat.com>> wrote:
> 
>     QEMU does not compile when enabling clang's thread safety analysis
>     (TSA),
>     because some functions create wrappers for pthread mutexes but do
>     not use any TSA macro. Therefore the compiler fails.
> 
>     In order to make the compiler happy and avoid adding all the
>     necessary macros to all callers (lock functions should use
>     TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers
>     of pthread_mutex_lock/pthread_mutex_unlock),
>     simply use TSA_NO_TSA to supppress such warnings.
> 
> 
> I'm not sure I understand this quite right. Maybe a clarifying question
> will help me understand: Why is this needed for bsd-user but not
> linux-user? How are they different here?
> 
Honestly I just went and fix the warnings that the compiler was throwing
at me. On FreeBSD it complains on bsd-user/mmap.c, but apparently the CI
or even compiling locally in linux doesn't create any issue with linux-user.

Weird. But I guess it won't hurt to add TSA_NO_TSA there too.

Emanuele

> Warner
>  
> 
>     Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com
>     <mailto:eesposit@redhat.com>>
>     ---
>      bsd-user/qemu.h         | 5 +++--
>      include/exec/exec-all.h | 5 +++--
>      2 files changed, 6 insertions(+), 4 deletions(-)
> 
>     diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
>     index be6105385e..711fdd1b64 100644
>     --- a/bsd-user/qemu.h
>     +++ b/bsd-user/qemu.h
>     @@ -37,6 +37,7 @@ extern char **environ;
>      #include "target_os_signal.h"
>      #include "target.h"
>      #include "exec/gdbstub.h"
>     +#include "qemu/clang-tsa.h"
> 
>      /*
>       * This struct is used to hold certain information about the
>     image.  Basically,
>     @@ -235,8 +236,8 @@ int target_msync(abi_ulong start, abi_ulong len,
>     int flags);
>      extern unsigned long last_brk;
>      extern abi_ulong mmap_next_start;
>      abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size);
>     -void mmap_fork_start(void);
>     -void mmap_fork_end(int child);
>     +void TSA_NO_TSA mmap_fork_start(void);
>     +void TSA_NO_TSA mmap_fork_end(int child);
> 
>      /* main.c */
>      extern char qemu_proc_pathname[];
>     diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>     index 25e11b0a8d..4f0c0559ac 100644
>     --- a/include/exec/exec-all.h
>     +++ b/include/exec/exec-all.h
>     @@ -25,6 +25,7 @@
>      #include "exec/cpu_ldst.h"
>      #endif
>      #include "qemu/interval-tree.h"
>     +#include "qemu/clang-tsa.h"
> 
>      /* allow to see translation results - the slowdown should be
>     negligible, so we leave it */
>      #define DEBUG_DISAS
>     @@ -758,8 +759,8 @@ static inline tb_page_addr_t
>     get_page_addr_code(CPUArchState *env,
>      }
> 
>      #if defined(CONFIG_USER_ONLY)
>     -void mmap_lock(void);
>     -void mmap_unlock(void);
>     +void TSA_NO_TSA mmap_lock(void);
>     +void TSA_NO_TSA mmap_unlock(void);
>      bool have_mmap_lock(void);
> 
>      /**
>     -- 
>     2.39.0
> 

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Kevin Wolf]

Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> eesposit@redhat.com> wrote:
> 
> > QEMU does not compile when enabling clang's thread safety analysis
> > (TSA),
> > because some functions create wrappers for pthread mutexes but do
> > not use any TSA macro. Therefore the compiler fails.
> >
> > In order to make the compiler happy and avoid adding all the
> > necessary macros to all callers (lock functions should use
> > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> > pthread_mutex_lock/pthread_mutex_unlock),
> > simply use TSA_NO_TSA to supppress such warnings.
> 
> I'm not sure I understand this quite right. Maybe a clarifying question
> will help me understand: Why is this needed for bsd-user but not
> linux-user? How are they different here?

FreeBSD's pthread headers include TSA annotations for some functions
that force us to do something about them (for now: suppress the warnings
in their callers) before we can enable -Wthread-safety for the purposes
where we really want it. Without this, calling functions like
pthread_mutex_lock() would cause compiler errors.

glibc's headers don't contain such annotations, so the same is not
necessary on Linux.

Kevin

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> > eesposit@redhat.com> wrote:
> >
> > > QEMU does not compile when enabling clang's thread safety analysis
> > > (TSA),
> > > because some functions create wrappers for pthread mutexes but do
> > > not use any TSA macro. Therefore the compiler fails.
> > >
> > > In order to make the compiler happy and avoid adding all the
> > > necessary macros to all callers (lock functions should use
> > > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> > > pthread_mutex_lock/pthread_mutex_unlock),
> > > simply use TSA_NO_TSA to supppress such warnings.
> >
> > I'm not sure I understand this quite right. Maybe a clarifying question
> > will help me understand: Why is this needed for bsd-user but not
> > linux-user? How are they different here?
>
> FreeBSD's pthread headers include TSA annotations for some functions
> that force us to do something about them (for now: suppress the warnings
> in their callers) before we can enable -Wthread-safety for the purposes
> where we really want it. Without this, calling functions like
> pthread_mutex_lock() would cause compiler errors.
>
> glibc's headers don't contain such annotations, so the same is not
> necessary on Linux
>

Thanks Kevin. With that explanation, these patches and their explanation
make perfect sense now. Often when there's a patch to bsd-user but not
linux-user, it's because bsd-user needs to do more in some way (which I try
to keep up on).

In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
understand why it's needed, and what I need to do next (though I think that
I may have to wait for the rest of qemu to be annotated)...

It might be better, though, to put some of this information in the commit
message so it isn't just on the mailing list. Just a suggestion:

Reviewed-by: Warner Losh <imp@bsdimp.com>

[-- Attachment #2: Type: text/html, Size: 2730 bytes --]

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Kevin Wolf]

Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> > > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> > > eesposit@redhat.com> wrote:
> > >
> > > > QEMU does not compile when enabling clang's thread safety analysis
> > > > (TSA),
> > > > because some functions create wrappers for pthread mutexes but do
> > > > not use any TSA macro. Therefore the compiler fails.
> > > >
> > > > In order to make the compiler happy and avoid adding all the
> > > > necessary macros to all callers (lock functions should use
> > > > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> > > > pthread_mutex_lock/pthread_mutex_unlock),
> > > > simply use TSA_NO_TSA to supppress such warnings.
> > >
> > > I'm not sure I understand this quite right. Maybe a clarifying question
> > > will help me understand: Why is this needed for bsd-user but not
> > > linux-user? How are they different here?
> >
> > FreeBSD's pthread headers include TSA annotations for some functions
> > that force us to do something about them (for now: suppress the warnings
> > in their callers) before we can enable -Wthread-safety for the purposes
> > where we really want it. Without this, calling functions like
> > pthread_mutex_lock() would cause compiler errors.
> >
> > glibc's headers don't contain such annotations, so the same is not
> > necessary on Linux
> >
> 
> Thanks Kevin. With that explanation, these patches and their explanation
> make perfect sense now. Often when there's a patch to bsd-user but not
> linux-user, it's because bsd-user needs to do more in some way (which I try
> to keep up on).
> 
> In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
> understand why it's needed, and what I need to do next (though I think that
> I may have to wait for the rest of qemu to be annotated)...

I assume that the bsd-user part is actually sufficiently independent
that you could do proper annotations there if you want.

However, be aware that TSA has some serious limitations with C, so you
can't express certain things, and it isn't as strict as it could be (in
particular, function pointers bypass it). As long as you have global
locks (as opposed to locks in structs), it kind of works, though.
Certainly better than nothing.

But it probably means that some of the rest of QEMU may never get the
annotations. Also, our primary goal is protecting the block layer, so
someone else would have to work on other locks. With checks disabled on
individual functions like in this series, it should at least be possible
to work on it incrementally.

> It might be better, though, to put some of this information in the commit
> message so it isn't just on the mailing list.

Yes, I agree. We can tweak the commit messages before merging it.

> Just a suggestion:
> 
> Reviewed-by: Warner Losh <imp@bsdimp.com>

Thanks!

Kevin

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Stefan Hajnoczi]

On Tue, 17 Jan 2023 at 12:17, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> > On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > > Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> > > > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> > > > eesposit@redhat.com> wrote:
> > > >
> > > > > QEMU does not compile when enabling clang's thread safety analysis
> > > > > (TSA),
> > > > > because some functions create wrappers for pthread mutexes but do
> > > > > not use any TSA macro. Therefore the compiler fails.
> > > > >
> > > > > In order to make the compiler happy and avoid adding all the
> > > > > necessary macros to all callers (lock functions should use
> > > > > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> > > > > pthread_mutex_lock/pthread_mutex_unlock),
> > > > > simply use TSA_NO_TSA to supppress such warnings.
> > > >
> > > > I'm not sure I understand this quite right. Maybe a clarifying question
> > > > will help me understand: Why is this needed for bsd-user but not
> > > > linux-user? How are they different here?
> > >
> > > FreeBSD's pthread headers include TSA annotations for some functions
> > > that force us to do something about them (for now: suppress the warnings
> > > in their callers) before we can enable -Wthread-safety for the purposes
> > > where we really want it. Without this, calling functions like
> > > pthread_mutex_lock() would cause compiler errors.
> > >
> > > glibc's headers don't contain such annotations, so the same is not
> > > necessary on Linux
> > >
> >
> > Thanks Kevin. With that explanation, these patches and their explanation
> > make perfect sense now. Often when there's a patch to bsd-user but not
> > linux-user, it's because bsd-user needs to do more in some way (which I try
> > to keep up on).
> >
> > In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
> > understand why it's needed, and what I need to do next (though I think that
> > I may have to wait for the rest of qemu to be annotated)...
>
> I assume that the bsd-user part is actually sufficiently independent
> that you could do proper annotations there if you want.
>
> However, be aware that TSA has some serious limitations with C, so you
> can't express certain things, and it isn't as strict as it could be (in
> particular, function pointers bypass it). As long as you have global
> locks (as opposed to locks in structs), it kind of works, though.
> Certainly better than nothing.

What are the limitations on locks in structs (a common case)?

Stefan

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Kevin Wolf]

Am 17.01.2023 um 21:43 hat Stefan Hajnoczi geschrieben:
> On Tue, 17 Jan 2023 at 12:17, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> > > On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > > Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> > > > > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> > > > > eesposit@redhat.com> wrote:
> > > > >
> > > > > > QEMU does not compile when enabling clang's thread safety analysis
> > > > > > (TSA),
> > > > > > because some functions create wrappers for pthread mutexes but do
> > > > > > not use any TSA macro. Therefore the compiler fails.
> > > > > >
> > > > > > In order to make the compiler happy and avoid adding all the
> > > > > > necessary macros to all callers (lock functions should use
> > > > > > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> > > > > > pthread_mutex_lock/pthread_mutex_unlock),
> > > > > > simply use TSA_NO_TSA to supppress such warnings.
> > > > >
> > > > > I'm not sure I understand this quite right. Maybe a clarifying question
> > > > > will help me understand: Why is this needed for bsd-user but not
> > > > > linux-user? How are they different here?
> > > >
> > > > FreeBSD's pthread headers include TSA annotations for some functions
> > > > that force us to do something about them (for now: suppress the warnings
> > > > in their callers) before we can enable -Wthread-safety for the purposes
> > > > where we really want it. Without this, calling functions like
> > > > pthread_mutex_lock() would cause compiler errors.
> > > >
> > > > glibc's headers don't contain such annotations, so the same is not
> > > > necessary on Linux
> > > >
> > >
> > > Thanks Kevin. With that explanation, these patches and their explanation
> > > make perfect sense now. Often when there's a patch to bsd-user but not
> > > linux-user, it's because bsd-user needs to do more in some way (which I try
> > > to keep up on).
> > >
> > > In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
> > > understand why it's needed, and what I need to do next (though I think that
> > > I may have to wait for the rest of qemu to be annotated)...
> >
> > I assume that the bsd-user part is actually sufficiently independent
> > that you could do proper annotations there if you want.
> >
> > However, be aware that TSA has some serious limitations with C, so you
> > can't express certain things, and it isn't as strict as it could be (in
> > particular, function pointers bypass it). As long as you have global
> > locks (as opposed to locks in structs), it kind of works, though.
> > Certainly better than nothing.
> 
> What are the limitations on locks in structs (a common case)?

TSA_GUARDED_BY() can't refer to a mutex in the same struct in C. You
would have to have something like 'this', but it just doesn't exist. (I
think in C++ you don't actually need 'this' because name resolution
automatically starts at the struct or something - I neither know C++
well enough nor TSA with it, so take this with a grain of salt.)

You can still annotate functions for such structs in C, because then you
have a name for the struct, like this:

void lock(Foo *foo) TSA_REQUIRES(foo->mutex);

Kevin

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]

On Wed, Jan 18, 2023, 04:15 Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.01.2023 um 21:43 hat Stefan Hajnoczi geschrieben:
> > On Tue, 17 Jan 2023 at 12:17, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> > > > On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > > >
> > > > > Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> > > > > > On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> > > > > > eesposit@redhat.com> wrote:
> > > > > >
> > > > > > > QEMU does not compile when enabling clang's thread safety
> analysis
> > > > > > > (TSA),
> > > > > > > because some functions create wrappers for pthread mutexes but
> do
> > > > > > > not use any TSA macro. Therefore the compiler fails.
> > > > > > >
> > > > > > > In order to make the compiler happy and avoid adding all the
> > > > > > > necessary macros to all callers (lock functions should use
> > > > > > > TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to
> allusers of
> > > > > > > pthread_mutex_lock/pthread_mutex_unlock),
> > > > > > > simply use TSA_NO_TSA to supppress such warnings.
> > > > > >
> > > > > > I'm not sure I understand this quite right. Maybe a clarifying
> question
> > > > > > will help me understand: Why is this needed for bsd-user but not
> > > > > > linux-user? How are they different here?
> > > > >
> > > > > FreeBSD's pthread headers include TSA annotations for some
> functions
> > > > > that force us to do something about them (for now: suppress the
> warnings
> > > > > in their callers) before we can enable -Wthread-safety for the
> purposes
> > > > > where we really want it. Without this, calling functions like
> > > > > pthread_mutex_lock() would cause compiler errors.
> > > > >
> > > > > glibc's headers don't contain such annotations, so the same is not
> > > > > necessary on Linux
> > > > >
> > > >
> > > > Thanks Kevin. With that explanation, these patches and their
> explanation
> > > > make perfect sense now. Often when there's a patch to bsd-user but
> not
> > > > linux-user, it's because bsd-user needs to do more in some way
> (which I try
> > > > to keep up on).
> > > >
> > > > In this case, it's because FreeBSD's libc is a bit ahead of the
> curve. So I
> > > > understand why it's needed, and what I need to do next (though I
> think that
> > > > I may have to wait for the rest of qemu to be annotated)...
> > >
> > > I assume that the bsd-user part is actually sufficiently independent
> > > that you could do proper annotations there if you want.
> > >
> > > However, be aware that TSA has some serious limitations with C, so you
> > > can't express certain things, and it isn't as strict as it could be (in
> > > particular, function pointers bypass it). As long as you have global
> > > locks (as opposed to locks in structs), it kind of works, though.
> > > Certainly better than nothing.
> >
> > What are the limitations on locks in structs (a common case)?
>
> TSA_GUARDED_BY() can't refer to a mutex in the same struct in C. You
> would have to have something like 'this', but it just doesn't exist. (I
> think in C++ you don't actually need 'this' because name resolution
> automatically starts at the struct or something - I neither know C++
> well enough nor TSA with it, so take this with a grain of salt.)
>
> You can still annotate functions for such structs in C, because then you
> have a name for the struct, like this:
>
> void lock(Foo *foo) TSA_REQUIRES(foo->mutex);
>

Thanks for the explanation!

Stefan

>

[-- Attachment #2: Type: text/html, Size: 5161 bytes --]

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Emanuele Giuseppe Esposito]

Am 17/01/2023 um 18:17 schrieb Kevin Wolf:
> Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
>> On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
>>>> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
>>>> eesposit@redhat.com> wrote:
>>>>
>>>>> QEMU does not compile when enabling clang's thread safety analysis
>>>>> (TSA),
>>>>> because some functions create wrappers for pthread mutexes but do
>>>>> not use any TSA macro. Therefore the compiler fails.
>>>>>
>>>>> In order to make the compiler happy and avoid adding all the
>>>>> necessary macros to all callers (lock functions should use
>>>>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
>>>>> pthread_mutex_lock/pthread_mutex_unlock),
>>>>> simply use TSA_NO_TSA to supppress such warnings.
>>>>
>>>> I'm not sure I understand this quite right. Maybe a clarifying question
>>>> will help me understand: Why is this needed for bsd-user but not
>>>> linux-user? How are they different here?
>>>
>>> FreeBSD's pthread headers include TSA annotations for some functions
>>> that force us to do something about them (for now: suppress the warnings
>>> in their callers) before we can enable -Wthread-safety for the purposes
>>> where we really want it. Without this, calling functions like
>>> pthread_mutex_lock() would cause compiler errors.
>>>
>>> glibc's headers don't contain such annotations, so the same is not
>>> necessary on Linux
>>>
>>
>> Thanks Kevin. With that explanation, these patches and their explanation
>> make perfect sense now. Often when there's a patch to bsd-user but not
>> linux-user, it's because bsd-user needs to do more in some way (which I try
>> to keep up on).
>>
>> In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
>> understand why it's needed, and what I need to do next (though I think that
>> I may have to wait for the rest of qemu to be annotated)...
> 
> I assume that the bsd-user part is actually sufficiently independent
> that you could do proper annotations there if you want.
> 
> However, be aware that TSA has some serious limitations with C, so you
> can't express certain things, and it isn't as strict as it could be (in
> particular, function pointers bypass it). As long as you have global
> locks (as opposed to locks in structs), it kind of works, though.
> Certainly better than nothing.
> 
> But it probably means that some of the rest of QEMU may never get the
> annotations. Also, our primary goal is protecting the block layer, so
> someone else would have to work on other locks. With checks disabled on
> individual functions like in this series, it should at least be possible
> to work on it incrementally.
> 
>> It might be better, though, to put some of this information in the commit
>> message so it isn't just on the mailing list.
> 
> Yes, I agree. We can tweak the commit messages before merging it.

New proposed commit message:

bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings in FreeBSD

FreeBSD implements pthread headers using TSA (thread safety analysis)
annotations, therefore when an application is compiled with -Wthread-safety
there are some locking/annotation requirements that the user of the
pthread API has to follow.

This will also be the case in QEMU, since bsd-user/mmap.c uses the
pthread API. Therefore when building it with -Wthread-safety the
compiler will throw warnings because the functions are not properly
annotated. We need TSA to be enabled because it ensures
that the critical sections of an annotated variable are properly
locked.

In order to make the compiler happy and avoid adding all the
necessary macros to all callers (lock functions should use
TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
users of pthread_mutex_lock/pthread_mutex_unlock),
simply use TSA_NO_TSA to supppress such warnings.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Same message could be applied to patch 1, substituting bsd-user/mmap
with util/qemu-thread-posix.


Thank you,
Emanuele

> 
>> Just a suggestion:
>>
>> Reviewed-by: Warner Losh <imp@bsdimp.com>
> 
> Thanks!
> 
> Kevin
> 

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4478 bytes --]

On Wed, Jan 18, 2023 at 04:12:09PM +0100, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 17/01/2023 um 18:17 schrieb Kevin Wolf:
> > Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> >> On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> >>>> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> >>>> eesposit@redhat.com> wrote:
> >>>>
> >>>>> QEMU does not compile when enabling clang's thread safety analysis
> >>>>> (TSA),
> >>>>> because some functions create wrappers for pthread mutexes but do
> >>>>> not use any TSA macro. Therefore the compiler fails.
> >>>>>
> >>>>> In order to make the compiler happy and avoid adding all the
> >>>>> necessary macros to all callers (lock functions should use
> >>>>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of
> >>>>> pthread_mutex_lock/pthread_mutex_unlock),
> >>>>> simply use TSA_NO_TSA to supppress such warnings.
> >>>>
> >>>> I'm not sure I understand this quite right. Maybe a clarifying question
> >>>> will help me understand: Why is this needed for bsd-user but not
> >>>> linux-user? How are they different here?
> >>>
> >>> FreeBSD's pthread headers include TSA annotations for some functions
> >>> that force us to do something about them (for now: suppress the warnings
> >>> in their callers) before we can enable -Wthread-safety for the purposes
> >>> where we really want it. Without this, calling functions like
> >>> pthread_mutex_lock() would cause compiler errors.
> >>>
> >>> glibc's headers don't contain such annotations, so the same is not
> >>> necessary on Linux
> >>>
> >>
> >> Thanks Kevin. With that explanation, these patches and their explanation
> >> make perfect sense now. Often when there's a patch to bsd-user but not
> >> linux-user, it's because bsd-user needs to do more in some way (which I try
> >> to keep up on).
> >>
> >> In this case, it's because FreeBSD's libc is a bit ahead of the curve. So I
> >> understand why it's needed, and what I need to do next (though I think that
> >> I may have to wait for the rest of qemu to be annotated)...
> > 
> > I assume that the bsd-user part is actually sufficiently independent
> > that you could do proper annotations there if you want.
> > 
> > However, be aware that TSA has some serious limitations with C, so you
> > can't express certain things, and it isn't as strict as it could be (in
> > particular, function pointers bypass it). As long as you have global
> > locks (as opposed to locks in structs), it kind of works, though.
> > Certainly better than nothing.
> > 
> > But it probably means that some of the rest of QEMU may never get the
> > annotations. Also, our primary goal is protecting the block layer, so
> > someone else would have to work on other locks. With checks disabled on
> > individual functions like in this series, it should at least be possible
> > to work on it incrementally.
> > 
> >> It might be better, though, to put some of this information in the commit
> >> message so it isn't just on the mailing list.
> > 
> > Yes, I agree. We can tweak the commit messages before merging it.
> 
> New proposed commit message:
> 
> bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings in FreeBSD
> 
> FreeBSD implements pthread headers using TSA (thread safety analysis)
> annotations, therefore when an application is compiled with -Wthread-safety
> there are some locking/annotation requirements that the user of the
> pthread API has to follow.
> 
> This will also be the case in QEMU, since bsd-user/mmap.c uses the
> pthread API. Therefore when building it with -Wthread-safety the
> compiler will throw warnings because the functions are not properly
> annotated. We need TSA to be enabled because it ensures
> that the critical sections of an annotated variable are properly
> locked.
> 
> In order to make the compiler happy and avoid adding all the
> necessary macros to all callers (lock functions should use
> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
> users of pthread_mutex_lock/pthread_mutex_unlock),
> simply use TSA_NO_TSA to supppress such warnings.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Same message could be applied to patch 1, substituting bsd-user/mmap
> with util/qemu-thread-posix.

Looks good to me.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 4547 bytes --]

On Wed, Jan 18, 2023 at 8:12 AM Emanuele Giuseppe Esposito <
eesposit@redhat.com> wrote:

>
>
> Am 17/01/2023 um 18:17 schrieb Kevin Wolf:
> > Am 17.01.2023 um 17:43 hat Warner Losh geschrieben:
> >> On Tue, Jan 17, 2023 at 9:25 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 17.01.2023 um 17:16 hat Warner Losh geschrieben:
> >>>> On Tue, Jan 17, 2023 at 6:52 AM Emanuele Giuseppe Esposito <
> >>>> eesposit@redhat.com> wrote:
> >>>>
> >>>>> QEMU does not compile when enabling clang's thread safety analysis
> >>>>> (TSA),
> >>>>> because some functions create wrappers for pthread mutexes but do
> >>>>> not use any TSA macro. Therefore the compiler fails.
> >>>>>
> >>>>> In order to make the compiler happy and avoid adding all the
> >>>>> necessary macros to all callers (lock functions should use
> >>>>> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers
> of
> >>>>> pthread_mutex_lock/pthread_mutex_unlock),
> >>>>> simply use TSA_NO_TSA to supppress such warnings.
> >>>>
> >>>> I'm not sure I understand this quite right. Maybe a clarifying
> question
> >>>> will help me understand: Why is this needed for bsd-user but not
> >>>> linux-user? How are they different here?
> >>>
> >>> FreeBSD's pthread headers include TSA annotations for some functions
> >>> that force us to do something about them (for now: suppress the
> warnings
> >>> in their callers) before we can enable -Wthread-safety for the purposes
> >>> where we really want it. Without this, calling functions like
> >>> pthread_mutex_lock() would cause compiler errors.
> >>>
> >>> glibc's headers don't contain such annotations, so the same is not
> >>> necessary on Linux
> >>>
> >>
> >> Thanks Kevin. With that explanation, these patches and their explanation
> >> make perfect sense now. Often when there's a patch to bsd-user but not
> >> linux-user, it's because bsd-user needs to do more in some way (which I
> try
> >> to keep up on).
> >>
> >> In this case, it's because FreeBSD's libc is a bit ahead of the curve.
> So I
> >> understand why it's needed, and what I need to do next (though I think
> that
> >> I may have to wait for the rest of qemu to be annotated)...
> >
> > I assume that the bsd-user part is actually sufficiently independent
> > that you could do proper annotations there if you want.
> >
> > However, be aware that TSA has some serious limitations with C, so you
> > can't express certain things, and it isn't as strict as it could be (in
> > particular, function pointers bypass it). As long as you have global
> > locks (as opposed to locks in structs), it kind of works, though.
> > Certainly better than nothing.
> >
> > But it probably means that some of the rest of QEMU may never get the
> > annotations. Also, our primary goal is protecting the block layer, so
> > someone else would have to work on other locks. With checks disabled on
> > individual functions like in this series, it should at least be possible
> > to work on it incrementally.
> >
> >> It might be better, though, to put some of this information in the
> commit
> >> message so it isn't just on the mailing list.
> >
> > Yes, I agree. We can tweak the commit messages before merging it.
>
> New proposed commit message:
>
> bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings in FreeBSD
>
> FreeBSD implements pthread headers using TSA (thread safety analysis)
> annotations, therefore when an application is compiled with -Wthread-safety
> there are some locking/annotation requirements that the user of the
> pthread API has to follow.
>
> This will also be the case in QEMU, since bsd-user/mmap.c uses the
> pthread API. Therefore when building it with -Wthread-safety the
> compiler will throw warnings because the functions are not properly
> annotated. We need TSA to be enabled because it ensures
> that the critical sections of an annotated variable are properly
> locked.
>
> In order to make the compiler happy and avoid adding all the
> necessary macros to all callers (lock functions should use
> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to all
> users of pthread_mutex_lock/pthread_mutex_unlock),
> simply use TSA_NO_TSA to supppress such warnings.
>

Looks good to me.

Warner


> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> Same message could be applied to patch 1, substituting bsd-user/mmap
> with util/qemu-thread-posix.
>
>
> Thank you,
> Emanuele
>
> >
> >> Just a suggestion:
> >>
> >> Reviewed-by: Warner Losh <imp@bsdimp.com>
> >
> > Thanks!
> >
> > Kevin
> >
>
>

[-- Attachment #2: Type: text/html, Size: 6160 bytes --]

Re: [PATCH 2/3] bsd-user/mmap: use TSA_NO_TSA to suppress clang TSA warnings

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On Tue, Jan 17, 2023 at 08:52:02AM -0500, Emanuele Giuseppe Esposito wrote:
> QEMU does not compile when enabling clang's thread safety analysis
> (TSA),
> because some functions create wrappers for pthread mutexes but do
> not use any TSA macro. Therefore the compiler fails.
> 
> In order to make the compiler happy and avoid adding all the
> necessary macros to all callers (lock functions should use
> TSA_ACQUIRE, while unlock TSA_RELEASE, and this applies to allusers of pthread_mutex_lock/pthread_mutex_unlock),
> simply use TSA_NO_TSA to supppress such warnings.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  bsd-user/qemu.h         | 5 +++--
>  include/exec/exec-all.h | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)

As a TSA newbie I'm wondering how would we go about annotating this
properly?

Maybe:
1. mmap_lock() ACQUIRE(), mmap_unlock() RELEASE()
2. Find all functions that call mmap_lock() but not mmap_release() and
   add ACQUIRE().
3. Find all functions that call mmap_unlock() but not mmap_lock() and
   add RELEASE().

Can you add an item to https://wiki.qemu.org/BiteSizedTasks so someone
who wants to spend a few hours auditing the code can do this?

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] configure: Enable -Wthread-safety if present

[Emanuele Giuseppe Esposito]

From: Kevin Wolf <kwolf@redhat.com>

This enables clang's thread safety analysis (TSA), which we'll use to
statically check the block graph locking.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20221207131838.239125-9-kwolf@redhat.com>
Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 2281892657..14668e6269 100755
--- a/configure
+++ b/configure
@@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
 add_to warn_flags -Wendif-labels
 add_to warn_flags -Wexpansion-to-defined
 add_to warn_flags -Wimplicit-fallthrough=2
+add_to warn_flags -Wthread-safety
 
 nowarn_flags=
 add_to nowarn_flags -Wno-initializer-overrides
-- 
2.39.0

Re: [PATCH 3/3] configure: Enable -Wthread-safety if present

[Daniel P. Berrangé]

On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> This enables clang's thread safety analysis (TSA), which we'll use to
> statically check the block graph locking.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20221207131838.239125-9-kwolf@redhat.com>
> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index 2281892657..14668e6269 100755
> --- a/configure
> +++ b/configure
> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
>  add_to warn_flags -Wendif-labels
>  add_to warn_flags -Wexpansion-to-defined
>  add_to warn_flags -Wimplicit-fallthrough=2
> +add_to warn_flags -Wthread-safety

Does this thread safety analysis have any kind of measurable
impact on compilation speed ?

Our CI jobs are quite sensitive to any increase in build
time.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] configure: Enable -Wthread-safety if present

[Emanuele Giuseppe Esposito]

Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
>> From: Kevin Wolf <kwolf@redhat.com>
>>
>> This enables clang's thread safety analysis (TSA), which we'll use to
>> statically check the block graph locking.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Message-Id: <20221207131838.239125-9-kwolf@redhat.com>
>> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  configure | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/configure b/configure
>> index 2281892657..14668e6269 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
>>  add_to warn_flags -Wendif-labels
>>  add_to warn_flags -Wexpansion-to-defined
>>  add_to warn_flags -Wimplicit-fallthrough=2
>> +add_to warn_flags -Wthread-safety
> 
> Does this thread safety analysis have any kind of measurable
> impact on compilation speed ?
> 
> Our CI jobs are quite sensitive to any increase in build
> time.

From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):

without clang:
real    2m46.729s\x0f
\x03user    19m24.122s\x0f
\x03sys     2m58.643s\x0f

with clang:
real    2m45.204s
user    19m52.096s
sys     2m9.036s

So there should be no significative impact.

I also forgot to mention that this serie fixes the CI failure seen in:

https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
https://gitlab.com/qemu-project/qemu/-/jobs/3479763746

Thank you,
Emanuele
> 
> 
> With regards,
> Daniel

Re: [PATCH 3/3] configure: Enable -Wthread-safety if present

[Daniel P. Berrangé]

On Tue, Jan 17, 2023 at 03:41:29PM +0100, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> > On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> >> From: Kevin Wolf <kwolf@redhat.com>
> >>
> >> This enables clang's thread safety analysis (TSA), which we'll use to
> >> statically check the block graph locking.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> Message-Id: <20221207131838.239125-9-kwolf@redhat.com>
> >> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  configure | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/configure b/configure
> >> index 2281892657..14668e6269 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
> >>  add_to warn_flags -Wendif-labels
> >>  add_to warn_flags -Wexpansion-to-defined
> >>  add_to warn_flags -Wimplicit-fallthrough=2
> >> +add_to warn_flags -Wthread-safety
> > 
> > Does this thread safety analysis have any kind of measurable
> > impact on compilation speed ?
> > 
> > Our CI jobs are quite sensitive to any increase in build
> > time.
> 
> From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
> Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):
> 
> without clang:
> real    2m46.729s\x0f
> \x03user    19m24.122s\x0f
> \x03sys     2m58.643s\x0f
> 
> with clang:
> real    2m45.204s
> user    19m52.096s
> sys     2m9.036s
> 
> So there should be no significative impact.
> 
> I also forgot to mention that this serie fixes the CI failure seen in:
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
> https://gitlab.com/qemu-project/qemu/-/jobs/3479763746

Odd, that job already has  -Wthread-safety included in CFLAGS, which
would seem to make this patch redundant, but I don't see where
-Wthread-safety came from in that pipeline 


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] configure: Enable -Wthread-safety if present

[Kevin Wolf]

Am 17.01.2023 um 16:01 hat Daniel P. Berrangé geschrieben:
> On Tue, Jan 17, 2023 at 03:41:29PM +0100, Emanuele Giuseppe Esposito wrote:
> > 
> > 
> > Am 17/01/2023 um 15:02 schrieb Daniel P. Berrangé:
> > > On Tue, Jan 17, 2023 at 08:52:03AM -0500, Emanuele Giuseppe Esposito wrote:
> > >> From: Kevin Wolf <kwolf@redhat.com>
> > >>
> > >> This enables clang's thread safety analysis (TSA), which we'll use to
> > >> statically check the block graph locking.
> > >>
> > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > >> Message-Id: <20221207131838.239125-9-kwolf@redhat.com>
> > >> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > >> ---
> > >>  configure | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/configure b/configure
> > >> index 2281892657..14668e6269 100755
> > >> --- a/configure
> > >> +++ b/configure
> > >> @@ -1183,6 +1183,7 @@ add_to warn_flags -Wnested-externs
> > >>  add_to warn_flags -Wendif-labels
> > >>  add_to warn_flags -Wexpansion-to-defined
> > >>  add_to warn_flags -Wimplicit-fallthrough=2
> > >> +add_to warn_flags -Wthread-safety
> > > 
> > > Does this thread safety analysis have any kind of measurable
> > > impact on compilation speed ?
> > > 
> > > Our CI jobs are quite sensitive to any increase in build
> > > time.
> > 
> > From a quick run in my machine (Dell PowerEdge R440 with Intel(R)
> > Xeon(R) Gold 5120 CPU @ 2.20GHz, 28 cpus):
> > 
> > without clang:
> > real    2m46.729s\x0f
> > \x03user    19m24.122s\x0f
> > \x03sys     2m58.643s\x0f
> > 
> > with clang:
> > real    2m45.204s
> > user    19m52.096s
> > sys     2m9.036s
> > 
> > So there should be no significative impact.
> > 
> > I also forgot to mention that this serie fixes the CI failure seen in:
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/3479763741
> > https://gitlab.com/qemu-project/qemu/-/jobs/3479763746
> 
> Odd, that job already has  -Wthread-safety included in CFLAGS, which
> would seem to make this patch redundant, but I don't see where
> -Wthread-safety came from in that pipeline 

This patch is what was already in the branch in which the above CI tests
ran. I dropped it from the pull request because of these build failures
on FreeBSD. (Their libc includes TSA annotations, which means that QEMU
functions have to be annotated as well when they call pthread locking
functions, before we can enable -Wthread-safety.)

So patches 1 and 2 fix the failure that you would otherwise see with
this patch.

Kevin

Re: [PATCH 0/3] TSA: make sure QEMU compiles when using clang TSA

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Tue, Jan 17, 2023 at 08:52:00AM -0500, Emanuele Giuseppe Esposito wrote:
> This serie aims to enable clang Thread Safety Analysis (TSA) in QEMU.

It's worth covering what TSA is and why it's useful:

Thread Safety Analysis "warns about potential race conditions in code.
The analysis is completely static (i.e. compile-time); there is no
run-time overhead"

"Thread safety analysis works very much like a type system for
multi-threaded programs. In addition to declaring the type of data (e.g.
int, float, etc.), the programmer can (optionally) declare how access to
that data is controlled in a multi-threaded environment. For example, if
foo is guarded by the mutex mu, then the analysis will issue a warning
whenever a piece of code reads or writes to foo without first locking
mu."

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] TSA: make sure QEMU compiles when using clang TSA

[Kevin Wolf]

Am 17.01.2023 um 14:52 hat Emanuele Giuseppe Esposito geschrieben:
> This serie aims to enable clang Thread Safety Analysis (TSA) in QEMU.
> The goal is to use it for our multiqueue project aiming to replace the
> block layer AioContext lock with a rwlock and make sure the lock is taken
> correctly everywhere [1].
> 
> By default, TSA covers pthread mutexes, therefore when added in QEMU it
> immediately detects some wrappers using pthread_mutex_lock/unlock without
> using the proper TSA macros. Since adding such macro requires scanning all
> possible callers of the affected wrapper, simply use TSA_NO_TSA to suppress
> the warnings.
> 
> [1] = https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg00903.html

Thanks, changed the commit messages as discussed and applied to my block
branch.

Kevin