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

[Kevin Wolf <kwolf@redhat.com>]

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