Clang thread safety analysis (many iterations)

Prerequisites:

Use CLang for builds. Add -Wthread-safety to compile options.

CLang now available also on Windows, m.b. installed as part of Visual Studio.

Process:

Add thread analysis annotations to the code.
Compile, investigate warnings
Refactor necessary snippets to fix the warnings. Several of them m.b. errors.
Repeat it with small changes to achieve more accurate and clean code.

This is long-term issue which can be done in hour; it is repeated task to be performed again and again with small changes, each of them make code cleaner.

Results:

More clean codeflow; catching possible bugs (for example, if you forget to take a lock before access a resource). If you use clang on regular basis, it will be immediately (just look to new warnings).

Code will covered by annotations which themselves works as quite good comments. (you don't need to step into a function guessing if it uses shared resources or not; just look at it's declaration for annotations).

Example:

Original snippet from MultiAgentDesc_t declaration:

    mutable CSphRwlock  m_dWeightLock; 
    CSphFixedVector<WORD>   m_dWeights {0}  /// the weights of the hosts

Actually m_dWeightLock added there to manage acces to m_dWeights. Well, let's comment it:

    mutable CSphRwlock  m_dWeightLock;      /// manages access to m_pWeights 
    CSphFixedVector<WORD>   m_dWeights {0}  /// the weights of the hosts

Oops.. A typo... Well, let's now annotate it

    mutable CSphRwlock      m_dWeightLock;  /// manages access to m_pWeights 
    CSphFixedVector<WORD>   m_dWeights      /// the weights of the hosts 
            GUARDED_BY (m_dWeightLock) { 0 };

Now take a snippet from MultiAgentDesc_t::ChooseWeightedRandAgent and explicitly comment a lock to show how it works:

    //CSphScopedRLock tLock ( m_dWeightLock ); 
    DWORD uBound = m_dWeights[*pBestAgent];

Compile... Look here:

/sphinx/sphinx-dev/src/searchdha.cpp:322:17: warning: reading variable 'm_dWeights' requires 
 holding mutex 'm_dWeightLock' [-Wthread-safety-analysis] 
        DWORD uBound = m_dWeights[*pBestAgent]; 
                       ^

Clang just catched race condition, and we can fix it. Now roll back the commented lock, and warning dissapeared. That is basically enough to manage, but we can also mark function's declaration this way:

    void ChooseWeightedRandAgent ( int * pBestAgent, CSphVector<int> & dCandidates ) 
            EXCLUDES ( m_dWeightLock );

Or even this way:

    void ChooseWeightedRandAgent ( int * pBestAgent, CSphVector<int> & dCandidates ) 
            REQUIRES ( !m_dWeightLock );

Both annotations works, but second one is more annoying and require the extra flag -Wthread-safety-negative for the compiler.

Finally we have in class declaration the snippet:

    mutable CSphRwlock      m_dWeightLock;  /// manages access to m_pWeights 
    CSphFixedVector<WORD>   m_dWeights      /// the weights of the hosts 
            GUARDED_BY (m_dWeightLock) { 0 }; 
... 
    const AgentDesc_c & ChooseAgent () REQUIRES ( !m_dWeightLock ); 
... 
    CSphFixedVector<WORD> GetWeights () const REQUIRES ( !m_dWeightLock ) 
... 
    const AgentDesc_c & StDiscardDead () REQUIRES ( !m_dWeightLock ); 
    const AgentDesc_c & StLowErrors () REQUIRES ( !m_dWeightLock ); 
... 
    void ChooseWeightedRandAgent ( int * pBestAgent, CSphVector<int> & dCandidates ) REQUIRES ( !m_dWeightLock ); 
    void CheckRecalculateWeights ( const CSphFixedVector<int64_t> &dTimers ) REQUIRES ( !m_dWeightLock );

Note that annotations are visible directly in class declaration, and clearly annotate that lock must not be held when calling any of them. If we explicitly add, say, call of StLowErrors() in some place where lock is locked, compiler will warn us:

    CSphScopedRLock tLock ( m_dWeightLock ); 
    auto &m = StLowErrors ();

 warning: cannot call function 'StLowErrors' while mutex 'm_dWeightLock' is held [-Wthread-safety-analysis] 
        auto &m = StLowErrors (); 
                  ^

So, in this case both annotations and warning help to avoid deadlock in runtime.

nck-2 / test-rep

Clang thread safety analysis (many iterations) #1320