On Wed, Jan 18, 2023 at 8:12 AM Emanuele Giuseppe Esposito < eespo...@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 <kw...@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 < > >>>> eespo...@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 <eespo...@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 <i...@bsdimp.com> > > > > Thanks! > > > > Kevin > > > >
