Update net_plugin to support clang Thread Safety Analysis features

[greg7mdp]

Resolves #1230

Newer versions of clang support a Google developed tool called "Thread Safety Analysis". Here is the doc.

Thread safety analysis works very much like a type system for multi-threaded programs. In addition to declaring the type of data (e.g. int, float, etc.), the programmer can (optionally) declare how access to that data is controlled in a multi-threaded environment. For example:

#include "mutex.h"

class BankAccount {
private:
  Mutex mu;
  int   balance GUARDED_BY(mu);

  void depositImpl(int amount) {
    balance += amount;       // WARNING! Cannot write balance without locking mu.
  }

  void withdrawImpl(int amount) REQUIRES(mu) {
    balance -= amount;       // OK. Caller must have locked mu.
  }

public:
  void withdraw(int amount) {
    mu.Lock();
    withdrawImpl(amount);    // OK.  We've locked mu.
  }                          // WARNING!  Failed to unlock mu.

  void transferFrom(BankAccount& b, int amount) {
    mu.Lock();
    b.withdrawImpl(amount);  // WARNING!  Calling withdrawImpl() requires locking b.mu.
    depositImpl(amount);     // OK.  depositImpl() has no requirements.
    mu.Unlock();
  }
};

The analysis is completely static (i.e. compile-time); there is no run-time overhead. I think our code robustness would greatly benefit to have these checks occur whenever the code is compiled with clang. In addition, these attributes are a useful documentation of the expected access controls.

This PR adds the access annotations for net_plugin.

I had to change the calling parameters for sync_manager::request_next_chunk(). Unfortunately there is an issue in the thread safety analysis code which issues incorrect warnings when a class managing locks adopts a lock. I submitted a LLVM issue for this.

[heifner]

This whole file is unfortunate. I would have expected clang to provide this on the std mutex classes.
Actually it looks like it does: https://reviews.llvm.org/D14731
Can you just remove this fc wrapper.

[greg7mdp]

This is only if we use the clang libc++. Typically we build with gnu's libstdc++.

[heifner]

Then we don't need it. As long as we have one build that checks this, then we don't need these wrappers.

[greg7mdp]

I believe that by default, clang uses the system's default c++ library (see this, and this seems to be the case on my ubuntu system - using GNU's libstdc++), in which case we wouldn't be using LLVM's libc++ even when building with clang.

[heifner]

I think it would be worth a dedicated build to avoid having to provide these wrappers.

[spoonincode]

The endgame is that we run a clang+libc++ build as part of CI, so if that is sufficient I do agree these wrappers are annoying goop. I'm not entirely following if the latest clang+libc++ works though, for example what does this comment mean

In addition even libc++ does not consistently provide these attributes on the mutex objects.

[greg7mdp]

what does this comment mean

in libcxx, you can see that lock_guard has the attributes, but unique_lock doesn't.

[greg7mdp]

Also in mutex only scoped_lock has the attributes.

[spoonincode]

Then it may be worth reflecting on why the feature has been described as "mature enough to be deployed in an industrial setting" for over 6 years, yet LLVM has neglected to integrate it fully in to their own stdlib in that time.

[greg7mdp]

I think the expectation is that people will write their own wrappers. It is necessary anyways if you use clang with the default c++ stdlib from the system (could be gnu on ubuntu, Microsoft's on windows), so providing your own wrappers guarantees that you benefit from the Thread Safety Analysis when building with clang, regardless of which stdlib you use.

in the doc, they write:

Thread safety analysis can be used with any threading library, but it does require that the threading API be wrapped in classes and methods which have the appropriate annotations. The following code provides mutex.h as an example; these methods should be filled in to call the appropriate underlying implementation.

[heifner]

Is ci/cd not running because of the conflict or some other reason?

[spoonincode]

yeah conflicts prevent CI from running

[heifner]

The wrapping of shared_lock can come in when applied to whole repo.