Page MenuHomeFreeBSD

bhyve: Disable thread safety analysis by the compiler
ClosedPublic

Authored by markj on Nov 6 2022, 7:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 15 2024, 8:11 AM
Unknown Object (File)
Feb 15 2024, 8:10 AM
Unknown Object (File)
Feb 15 2024, 8:10 AM
Unknown Object (File)
Feb 15 2024, 7:13 AM
Unknown Object (File)
Dec 23 2023, 2:36 AM
Unknown Object (File)
Dec 12 2023, 3:25 PM
Unknown Object (File)
Sep 6 2023, 12:26 AM
Unknown Object (File)
Aug 23 2023, 10:35 AM

Details

Summary

I went through all of the thread safety warnings raised by clang but all
are false positives. Generally they arise from code which does
something like this:

    if (cond)
	    pthread_mutex_lock(m);
    ...
    if (cond)
	    pthread_mutex_unlock(m);

Or a more complicated example in pci_xhci_handle_transfer():

    retry:
    <usb xfer lock should be held here>
    ...
    if (!do_retry)
	    pthread_mutex_unlock(usb xfer lock);
    ...
    if (do_retry)
	    goto retry;

The lock_annotate() macro in cdefs.h is not very useful since it
seemingly can't be used to refer to function parameters, e.g.,
&sc->sc_mtx. To silence warnings I ended up having to annotate
functions with
no_lock_analysis.

In my opinion the warnings aren't very useful and won't catch anything
but the most straightforward locking bugs, and they probably won't even
do that since many functions which have some non-trivial locking pattern
will be annotated with __no_lock_analysis anyway. So let's just turn
them off.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Even if it only finds very simple bugs, it's a good option. Simple bugs happen. Setting up a VM to find a bug requires some work and is error prone. So, it's much better if the compiler complains about your bug.
Note that if we disable the option, it'll get even worse.

I haven't checked the warnings by myself yet. So, I don't know how useful this warning really is.

Btw: your xhci example can easily be resolved:

retry:
    <usb xfer lock should be held here>
    ...
    if (do_retry)
	    goto retry;
    pthread_mutex_unlock(usb xfer lock);

Even if it only finds very simple bugs, it's a good option. Simple bugs happen. Setting up a VM to find a bug requires some work and is error prone. So, it's much better if the compiler complains about your bug.
Note that if we disable the option, it'll get even worse.

The option is already disabled - if I'm not mistaken, it gets enabled when WARNS >= 6.

I haven't checked the warnings by myself yet. So, I don't know how useful this warning really is.

There's about 32 warnings that fall under this option.

I see no issue with keeping this option sidelined so that WARNS can be bumped to the default level.

In D37295#847410, @rew wrote:

I see no issue with keeping this option sidelined so that WARNS can be bumped to the default level.

Yes, it's a very good step forward if we can bump WARNS to the default level.
The commit message and the code comment doesn't sound like it's intended to enable this option in the future. I just want to mention that we should think about it.

This revision is now accepted and ready to land.Nov 8 2022, 7:03 AM
In D37295#847410, @rew wrote:

I see no issue with keeping this option sidelined so that WARNS can be bumped to the default level.

Yes, it's a very good step forward if we can bump WARNS to the default level.
The commit message and the code comment doesn't sound like it's intended to enable this option in the future. I just want to mention that we should think about it.

I submitted D37360 for discussion.

This revision was automatically updated to reflect the committed changes.