bhyve: Enable thread safety warnings by default

[markj]

Summary

This is for discussion, I haven't tested the patch yet.

[markj]

usr.sbin/bhyve/mem.c

217

I don't really like the code duplication here, but I can't see a way to pacify the compiler here without using __no_lock_analysis.

usr.sbin/bhyve/pci_e82545.c

1464

I can't refer to sc->esc_mtx in the function attribute.

Maybe this function could instead be inlined into its sole caller.

usr.sbin/bhyve/pci_xhci.c

1877

I suppose it's always ok to call this function with the mutex held. I wasn't sure if the mutex was dropped earlier as a microoptimization or if it was required for correctness.

usr.sbin/bhyve/virtio.h

251

This is a bit strange but avoids having to pollute all of its consumers with __no_lock_analysis.

[corvink]

usr.sbin/bhyve/pci_e82545.c

1464

I prefer to avoid __no_lock_analysis as much as possible. Possible solutions would be:

1. inline the function
2. pass the mtx as additional parameter and use __locks_exclusive
3. maybe someone else has better ideas

usr.sbin/bhyve/pci_xhci.c

1877

I'm unfamiliar with usb. So, I don't know as well.
Nevertheless, the old code only calls this function with the mutex released when pci_xhci_xfer_complete fails. I think the change should be fine.

usr.sbin/bhyve/virtio.h

251

Why don't we make the mtx mandatory by removing the if statement? It shouldn't hurt (as far as I can see, currently all virtio emulation have a mtx), it allows us to use thread safety analysis and it makes the code clearer.

[jhb]

usr.sbin/bhyve/mem.c

217

You can at least limit the duplication slightly if you use immutable to guard the invocation of the callback rather than guarding when you release the lock:

if (!immutable)
    err = cb(...);

error = pthread_rwlock_unlock(...);
assert(error == 0);

if (immutable)
    err = cb(...);

That said, I actually think it is more readable to do what you've done here even though it duplicates the code a bit.

usr.sbin/bhyve/pci_e82545.c

1464

I think I prefer using the annotation it uses now. All the other choices make the code less readable. It seems like a silly compiler limitation that it can't handle sc->esc_mtx in the attribute. The annotations seem useless for non-toy code if it doesn't allow for locks being members of function parameters.

usr.sbin/bhyve/virtio.c

583

So the compiler is too dumb to follow this logic? Does it think vs_mtx could change in the two places?

usr.sbin/bhyve/virtio.h

251

Assuming you can't use a structure member just like in the e1000 driver, I don't think you can fix this to not need __no_lock_analysis

[markj]

usr.sbin/bhyve/pci_e82545.c

1464

Reading the clang docs (https://releases.llvm.org/14.0.0/tools/clang/docs/ThreadSafetyAnalysis.html), I think this analysis is really mostly useful in C++.

I don't particularly want to inline e82545_tx_run() either. It seems wrong to contort the code just to satisfy these checks, especially when we're not getting most of the benefit (warnings about unlocked accesses to struct fields, which *is* useful).

usr.sbin/bhyve/virtio.c

583

Yes, it seems so. This is really similar to the case in mem.c, where even something like

const bool immutable = <something>
if (immutable)
    unlock;
<something>
if (immutable)
    lock;

raises a warning.

[corvink]

usr.sbin/bhyve/pci_e82545.c

1464

Looks like most of this patch will add __no_lock_analysis to all locking function. So, this patch is mostly useless.

Due to our config and nvlist issue, bhyve may become a C++ program. In that case, we can benefit from such things like warnings about unlocked accesses to struct fields.

[markj]

usr.sbin/bhyve/pci_e82545.c

1464

In that case, we can benefit from such things like warnings about unlocked accesses to struct fields.

Well, this would require lots of refactoring to turn device models into classes. First I'll work on the nvlist problem, then let's see.

So do we agree that this patch should not be committed as it is? I would like to get the benefits of clang's lock analysis, but today it simply doesn't play nicely with bhyve's code style.

[jhb]

usr.sbin/bhyve/pci_e82545.c

1464

I agree with not committing this patch and just leaving the warnings disabled for now.

[corvink]

usr.sbin/bhyve/pci_e82545.c

1464

I agree too.

[corvink]

@markj Can you please close this review?