bhyve: Disable thread safety analysis by the compiler

[markj]

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.

[corvink]

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);

[rew]

In D37295#847262, @corvink wrote:

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.

[corvink]

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.

[markj]

In D37295#847562, @corvink wrote:

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.