Re: Your clang TSA patches

[Kevin Wolf <kwolf@redhat.com>]

Am 09.05.2023 um 09:00 hat Marc-André Lureau geschrieben:
> Hi Stefan
> 
> On Mon, May 8, 2023 at 6:54 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > Hi Marc-André,
> > clang's Thread Safety Analysis is being used more in the QEMU block
> > layer and a few limitations have been hit:
> > 1. Mutexes that are struct fields are not supported (only global mutexes
> > work).
> > 2. Analysis does not extend across function pointers. See
> > GRAPH_RDLOCK_PTR for a workaround that is currently used.
> >
> > (There was a third limitation that I don't remember. Maybe Kevin
> > remembers.)
> >
> > I heard you had clang or LLVM patches in that past that improve TSA.
> > Did your patches address these things? Any idea how hard it is to fix
> > these limitations?
> 
> I don't remember much about that work, I didn't spend that much time on it,
> and it was 5y ago already!
> 
> My WIP branch is still available:
> https://github.com/elmarco/clang/commits/qemu-ta
> 
> it seems I was trying to modify clang to support TSA annotations on
> typedef and function pointers, and warn on unmatching. I have no idea
> if this is still relevant. I remember dropping the effort at that time
> because of lack of interest or help in both projects.

TSA support for functions pointers would be very helpful for the work
we're currently doing in the context of multiqueue support. Function
pointers are essentially where the compiler checks break down for us and
we have to cover these parts manually during review. And we have
function pointers in almost every call path (mostly those in BlockDriver
these days, but also some callbacks passed to functions, like BHs).

Does "unmatching" mean that it checks that the TSA annotations in
prototypes and the actual function declaration agree? If so, that would
be nice, too. For now, I've settled on annotating only the header file
for public functions, which works, but it's harder to read.

The third limitation Stefan couldn't remember is lack of support for
__attribute__((cleanup)), which is used by the lock guard macros. This
one is pretty annoying and means that I'm using the guards less than I
would like.

Stefan's first point, not supporting mutexes in struct fields, is a
major limitation of TSA with C, too, but it doesn't actually affect the
current work in QEMU where we're dealing with a single global lock. Of
course, if it were supported, we would certainly have uses for that,
too.

Kevin