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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]