Add Clang Thread Safety Annotations

more docs

Document

fix locking in tests

CMakeLists.txt

@@ -36,7 +36,7 @@ set(CMAKE_ENABLE_EXPORTS TRUE)
 
 # Set warning compile flags
 set(C_WARNING_GNU -Wall -Wextra -Werror -Wpedantic -pedantic-errors -Wimplicit-fallthrough=3 -Wno-empty-body -Wno-unused-parameter  -Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits)
-set(C_WARNING_Clang -Wall -Wextra -Werror -Wpedantic -Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-language-extension-token)
+set(C_WARNING_Clang -Wall -Wextra -Werror -Wpedantic -Wthread-safety -Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-language-extension-token)
 
 set(C_WARNING_FLAGS ${C_WARNING_${CMAKE_C_COMPILER_ID}})
 add_compile_options(${C_WARNING_FLAGS})
@@ -67,22 +67,22 @@ if(NOT DEFINED VERSION)
                       OUTPUT_VARIABLE DEFAULT_VERSION
                       RESULT_VARIABLE GIT_RESULT
                       OUTPUT_STRIP_TRAILING_WHITESPACE)
-                      
-                
+
+
       # You might sometimes get a fatal error when running the above command (when this is not a git repo).
       # GIT_RESULT will contain the result of last child process. It will be zero if successful.
       if (GIT_RESULT EQUAL 0)
           # Git succeeded, we will use the DEFAULT_VERSION as the QPID_DISPATCH_VERSION
           set(QPID_DISPATCH_VERSION ${DEFAULT_VERSION})
        else()
-          # The git command failed, set QPID_DISPATCH_VERSION to "UNKNOWN" 
+          # The git command failed, set QPID_DISPATCH_VERSION to "UNKNOWN"
           set(QPID_DISPATCH_VERSION "UNKNOWN")
        endif(GIT_RESULT EQUAL 0)
    else(Git_FOUND)
        # Git executable was not available, we will not be able to determine the version, just set it to "UNKNOWN"
        set(QPID_DISPATCH_VERSION "UNKNOWN")
    endif(Git_FOUND)
-   
+
 else(NOT DEFINED VERSION)
 
    # What if VERSION is defined but someone passed in an empty value for VERSION? Deal with that case here.
@@ -95,7 +95,7 @@ else(NOT DEFINED VERSION)
       else()
           set(QPID_DISPATCH_VERSION ${VERSION})
       endif()
-   endif(VERSION STREQUAL "")  
+   endif(VERSION STREQUAL "")
 endif(NOT DEFINED VERSION)
 
 message(STATUS "Setting skupper-router version to ${QPID_DISPATCH_VERSION}")

decisions/lock checing.md

@@ -0,0 +1,46 @@
+# XXXX. Use static Thread Safety Analysis annotations
+
+Date: 2022-03-21
+
+## Status
+
+Proposed
+
+## Context
+
+Some mistakes in lock usage can be detected statically.
+As opposed to dynamic approaches, either TSan (already in use) or lock priorities (proposed in [2], not implemented), the static approach proposed now can be checked reliably in the compiler, or in the IDE (if based on clangd).
+
+The way it works is that functions are annotated with what locks they require to be held, and the compiler then checks these conditions are satisfied.
+
+Some additional features of the Clang solution, namely, the TA_GUARDED() annotation for struct members, are available only in C++ [3]; meaning these features may not be used in skupper-router. 
+
+[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
+[2] 
+
+## Decision
+
+Annotate all functions that acquire, release, or require locks with the Clang Thread Safety Analysis annotations.
+Run a CI job that will check these annotations.
+
+## Consequences
+
+Header file defining the annotations needs to be included.
+There are multiple possibilities, either the Clang one(mutex.h), or the Zircon one (thread_annotations.h).
+They are functionally equivalent, differing only in license and how they name the helper macros.
+
+Annotation checking (-W) is only implemented in Clang, the GCC implementation was abandoned.
+
+The lock order checking () is not implemented (not even in Clang)
+
+
+
+Disadvantages and limitations
+
+Python code is not visible to Clang
+
+limited C support
+
+requires clang
+
+? can also annotate for coverity ?

include/qpid/dispatch/internal/thread_annotations.h

@@ -0,0 +1,61 @@
+// Copyright 2017 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#pragma once
+
+// This header defines thread annotation macros to be used everywhere in Zircon
+// outside of publicly exposed headers. See system/public/zircon/compiler.h for
+// the publicly exported macros.
+
+// The thread safety analysis system is documented at
+// http://clang.llvm.org/docs/ThreadSafetyAnalysis.html and its use in Zircon is documented at
+// docs/thread_annotations.md.  The macros we use are:
+//
+// TA_CAP(x)                    |x| is the capability this type represents, e.g. "mutex".
+// TA_GUARDED(x)                the annotated variable is guarded by the capability (e.g. lock) |x|
+// TA_ACQ(x)                    function acquires the mutex |x|
+// TA_ACQ_SHARED(x)             function acquires the mutex |x| for shared reading
+// TA_ACQ_BEFORE(x)             Indicates that if both this mutex and muxex |x| are to be acquired,
+//                              that this mutex must be acquired before mutex |x|.
+// TA_ACQ_AFTER(x)              Indicates that if both this mutex and muxex |x| are to be acquired,
+//                              that this mutex must be acquired after mutex |x|.
+// TA_TRY_ACQ(bool, x)          function acquires the mutex |x| if the function returns |bool|
+// TA_TRY_ACQ_SHARED(bool, x)   function acquires the mutex |x| for shared reading if the function
+//                              returns |bool|
+// TA_REL(x)                    function releases the mutex |x|
+// TA_REL_SHARED(x)             function releases the shared for reading mutex |x|
+// TA_ASSERT(x)                 function asserts that |x| is held
+// TA_ASSERT_SHARED(x)          function asserts that |x| is held for shared reading
+// TA_REQ(x)                    function requires that the caller hold the mutex |x|
+// TA_REQ_SHARED(x)             function requires that the caller hold the mutex |x| for shared
+// reading TA_EXCL(x)                   function requires that the caller not be holding the mutex
+// |x| TA_RET_CAP(x)                function returns a reference to the mutex |x| TA_SCOPED_CAP type
+// represents a scoped or RAII-style wrapper around a capability TA_NO_THREAD_SAFETY_ANALYSIS
+// function is excluded entirely from thread safety analysis
+
+#ifdef __clang__
+#define THREAD_ANNOTATION(x) __attribute__((x))
+#else
+#define THREAD_ANNOTATION(x)
+#endif
+
+#define TA_CAP(x) THREAD_ANNOTATION(capability(x))
+#define TA_GUARDED(x) THREAD_ANNOTATION(guarded_by(x))
+#define TA_ACQ(...) THREAD_ANNOTATION(acquire_capability(__VA_ARGS__))
+#define TA_ACQ_SHARED(...) THREAD_ANNOTATION(acquire_shared_capability(__VA_ARGS__))
+#define TA_ACQ_BEFORE(...) THREAD_ANNOTATION(acquired_before(__VA_ARGS__))
+#define TA_ACQ_AFTER(...) THREAD_ANNOTATION(acquired_after(__VA_ARGS__))
+#define TA_TRY_ACQ(...) THREAD_ANNOTATION(try_acquire_capability(__VA_ARGS__))
+#define TA_TRY_ACQ_SHARED(...) THREAD_ANNOTATION(try_acquire_shared_capability(__VA_ARGS__))
+#define TA_REL(...) THREAD_ANNOTATION(release_capability(__VA_ARGS__))
+#define TA_REL_SHARED(...) THREAD_ANNOTATION(release_shared_capability(__VA_ARGS__))
+#define TA_REL_GENERIC(...) THREAD_ANNOTATION(release_generic_capability(__VA_ARGS__))
+#define TA_ASSERT(...) THREAD_ANNOTATION(assert_capability(__VA_ARGS__))
+#define TA_ASSERT_SHARED(...) THREAD_ANNOTATION(assert_shared_capability(__VA_ARGS__))
+#define TA_REQ(...) THREAD_ANNOTATION(requires_capability(__VA_ARGS__))
+#define TA_REQ_SHARED(...) THREAD_ANNOTATION(requires_shared_capability(__VA_ARGS__))
+#define TA_EXCL(...) THREAD_ANNOTATION(locks_excluded(__VA_ARGS__))
+#define TA_RET_CAP(x) THREAD_ANNOTATION(lock_returned(x))
+#define TA_SCOPED_CAP THREAD_ANNOTATION(scoped_lockable)
+#define TA_NO_THREAD_SAFETY_ANALYSIS THREAD_ANNOTATION(no_thread_safety_analysis)

include/qpid/dispatch/threading.h

@@ -23,30 +23,32 @@
  * Portable threading and locking API.
  */
 
-typedef struct sys_mutex_t sys_mutex_t;
+#include "qpid/dispatch/internal/thread_annotations.h"
+
+typedef struct sys_mutex_t TA_CAP("mutex") sys_mutex_t;
 
 sys_mutex_t *sys_mutex(void);
 void         sys_mutex_free(sys_mutex_t *mutex);
-void         sys_mutex_lock(sys_mutex_t *mutex);
-void         sys_mutex_unlock(sys_mutex_t *mutex);
+void         sys_mutex_lock(sys_mutex_t *mutex) TA_ACQ(*mutex);
+void         sys_mutex_unlock(sys_mutex_t *mutex) TA_REL(*mutex);
 
 
-typedef struct sys_cond_t sys_cond_t;
+typedef struct sys_cond_t TA_CAP("cond") sys_cond_t;
 
 sys_cond_t *sys_cond(void);
 void        sys_cond_free(sys_cond_t *cond);
-void        sys_cond_wait(sys_cond_t *cond, sys_mutex_t *held_mutex);
+void        sys_cond_wait(sys_cond_t *cond, sys_mutex_t *held_mutex) TA_REQ(*held_mutex);
 void        sys_cond_signal(sys_cond_t *cond);
 void        sys_cond_signal_all(sys_cond_t *cond);
 
 
-typedef struct sys_rwlock_t sys_rwlock_t;
+typedef struct sys_rwlock_t TA_CAP("rwlock") sys_rwlock_t;
 
 sys_rwlock_t *sys_rwlock(void);
 void          sys_rwlock_free(sys_rwlock_t *lock);
-void          sys_rwlock_wrlock(sys_rwlock_t *lock);
-void          sys_rwlock_rdlock(sys_rwlock_t *lock);
-void          sys_rwlock_unlock(sys_rwlock_t *lock);
+void          sys_rwlock_wrlock(sys_rwlock_t *lock) TA_ACQ(lock);
+void          sys_rwlock_rdlock(sys_rwlock_t *lock) TA_ACQ_SHARED(lock);
+void          sys_rwlock_unlock(sys_rwlock_t *lock) TA_REL_GENERIC(lock);
 
 
 typedef struct sys_thread_t sys_thread_t;

src/dispatch.c

@@ -374,8 +374,8 @@ void qd_dispatch_free(qd_dispatch_t *qd)
 }
 
 
-QD_EXPORT void qd_dispatch_router_lock(qd_dispatch_t *qd) { sys_mutex_lock(qd->router->lock); }
-QD_EXPORT void qd_dispatch_router_unlock(qd_dispatch_t *qd) { sys_mutex_unlock(qd->router->lock); }
+QD_EXPORT void qd_dispatch_router_lock(qd_dispatch_t *qd) TA_ACQ(qd->router->lock) { sys_mutex_lock(qd->router->lock); }
+QD_EXPORT void qd_dispatch_router_unlock(qd_dispatch_t *qd) TA_REL(qd->router->lock) { sys_mutex_unlock(qd->router->lock); }
 
 qdr_core_t* qd_dispatch_router_core(qd_dispatch_t *qd) {
     return qd->router->router_core;

src/entity_cache.c

@@ -83,7 +83,7 @@ void qd_entity_cache_remove(const char *type, void *object) { push_event(REMOVE,
 // Locks the entity cache so entities can be updated safely (prevent entities from being deleted.)
 // Do not process any entities if return error code != 0
 // Must call qd_entity_refresh_end when done, regardless of error code.
-QD_EXPORT qd_error_t qd_entity_refresh_begin(PyObject *list) {
+QD_EXPORT qd_error_t qd_entity_refresh_begin(PyObject *list) TA_ACQ(event_lock) TA_NO_THREAD_SAFETY_ANALYSIS {
     if (!event_lock) return QD_ERROR_NONE;    /* Unit tests don't call qd_entity_cache_initialize */
     qd_error_clear();
     sys_mutex_lock(event_lock);
@@ -101,6 +101,6 @@ QD_EXPORT qd_error_t qd_entity_refresh_begin(PyObject *list) {
     return qd_error_code();
 }
 
-QD_EXPORT void qd_entity_refresh_end() {
+QD_EXPORT void qd_entity_refresh_end() TA_REL(event_lock) {
     sys_mutex_unlock(event_lock);
 }

src/message.c

@@ -29,6 +29,7 @@
 #include "qpid/dispatch/amqp.h"
 #include "qpid/dispatch/ctools.h"
 #include "qpid/dispatch/error.h"
+#include "qpid/dispatch/internal/thread_annotations.h"
 #include "qpid/dispatch/iterator.h"
 #include "qpid/dispatch/log.h"
 #include "qpid/dispatch/threading.h"
@@ -2816,15 +2817,15 @@ void qd_message_Q2_holdoff_disable(qd_message_t *msg)
 }
 
 
-bool _Q2_holdoff_should_block_LH(const qd_message_content_t *content)
+bool _Q2_holdoff_should_block_LH(const qd_message_content_t *content) TA_REQ(content->lock)
 {
     const size_t buff_ct = DEQ_SIZE(content->buffers);
     assert(buff_ct >= content->protected_buffers);
     return !content->disable_q2_holdoff && (buff_ct - content->protected_buffers) >= QD_QLIMIT_Q2_UPPER;
 }
 
 
-bool _Q2_holdoff_should_unblock_LH(const qd_message_content_t *content)
+bool _Q2_holdoff_should_unblock_LH(const qd_message_content_t *content) TA_REQ(content->lock)
 {
     const size_t buff_ct = DEQ_SIZE(content->buffers);
     assert(buff_ct >= content->protected_buffers);

src/message_private.h

@@ -21,6 +21,7 @@
 
 #include "qpid/dispatch/alloc.h"
 #include "qpid/dispatch/atomic.h"
+#include "qpid/dispatch/internal/thread_annotations.h"
 #include "qpid/dispatch/message.h"
 #include "qpid/dispatch/threading.h"
 
@@ -185,8 +186,8 @@ void qd_message_initialize();
 //
 
 // These expect content->lock to be locked.
-bool _Q2_holdoff_should_block_LH(const qd_message_content_t *content);
-bool _Q2_holdoff_should_unblock_LH(const qd_message_content_t *content);
+bool _Q2_holdoff_should_block_LH(const qd_message_content_t *content) TA_REQ(content->lock);
+bool _Q2_holdoff_should_unblock_LH(const qd_message_content_t *content) TA_REQ(content->lock);
 
 uint32_t _compose_router_annotations(qd_message_pvt_t *msg, unsigned int ra_flags, qd_buffer_list_t *ra_buffers);
 

src/posix/threading.c

@@ -26,6 +26,7 @@
 #include "qpid/dispatch/threading.h"
 
 #include "qpid/dispatch/ctools.h"
+#include "qpid/dispatch/internal/thread_annotations.h"
 
 #include <assert.h>
 #include <pthread.h>
@@ -52,14 +53,14 @@ void sys_mutex_free(sys_mutex_t *mutex)
 }
 
 
-void sys_mutex_lock(sys_mutex_t *mutex)
+void sys_mutex_lock(sys_mutex_t *mutex) TA_ACQ(*mutex) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     int result = pthread_mutex_lock(&(mutex->mutex));
     assert(result == 0);
 }
 
 
-void sys_mutex_unlock(sys_mutex_t *mutex)
+void sys_mutex_unlock(sys_mutex_t *mutex) TA_REL(*mutex) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     int result = pthread_mutex_unlock(&(mutex->mutex));
     assert(result == 0);
@@ -87,7 +88,7 @@ void sys_cond_free(sys_cond_t *cond)
 }
 
 
-void sys_cond_wait(sys_cond_t *cond, sys_mutex_t *held_mutex)
+void sys_cond_wait(sys_cond_t *cond, sys_mutex_t *held_mutex) TA_REQ(*held_mutex)
 {
     int result = pthread_cond_wait(&(cond->cond), &(held_mutex->mutex));
     assert(result == 0);
@@ -128,21 +129,21 @@ void sys_rwlock_free(sys_rwlock_t *lock)
 }
 
 
-void sys_rwlock_wrlock(sys_rwlock_t *lock)
+void sys_rwlock_wrlock(sys_rwlock_t *lock) TA_ACQ(*lock) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     int result = pthread_rwlock_wrlock(&(lock->lock));
     assert(result == 0);
 }
 
 
-void sys_rwlock_rdlock(sys_rwlock_t *lock)
+void sys_rwlock_rdlock(sys_rwlock_t *lock) TA_ACQ_SHARED(*lock) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     int result = pthread_rwlock_rdlock(&(lock->lock));
     assert(result == 0);
 }
 
 
-void sys_rwlock_unlock(sys_rwlock_t *lock)
+void sys_rwlock_unlock(sys_rwlock_t *lock) TA_REL_GENERIC(*lock) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     int result = pthread_rwlock_unlock(&(lock->lock));
     assert(result == 0);

src/router_core/delivery.c

@@ -298,7 +298,7 @@ void qdr_delivery_reject_CT(qdr_core_t *core, qdr_delivery_t *dlv, qdr_error_t *
 }
 
 
-bool qdr_delivery_settled_CT(qdr_core_t *core, qdr_delivery_t *dlv)
+bool qdr_delivery_settled_CT(qdr_core_t *core, qdr_delivery_t *dlv) TA_NO_THREAD_SAFETY_ANALYSIS
 {
     //
     // Remove a delivery from its unsettled list.  Side effects include issuing

tests/message_test.c

@@ -616,17 +616,21 @@ static char* test_q2_input_holdoff_sensing(void *context)
     for (int nbufs=1; nbufs<QD_QLIMIT_Q2_UPPER + 1; nbufs++) {
         qd_message_t         *msg     = qd_message();
         qd_message_content_t *content = MSG_CONTENT(msg);
+        sys_mutex_lock(content->lock);
 
         set_content_bufs(content, nbufs);
         if (_Q2_holdoff_should_block_LH(content) != (nbufs >= QD_QLIMIT_Q2_UPPER)) {
+            sys_mutex_unlock(content->lock);
             qd_message_free(msg);
             return "qd_message_holdoff_would_block was miscalculated";
         }
         if (_Q2_holdoff_should_unblock_LH(content) != (nbufs < QD_QLIMIT_Q2_LOWER)) {
+            sys_mutex_unlock(content->lock);
             qd_message_free(msg);
             return "qd_message_holdoff_would_unblock was miscalculated";
         }
 
+        sys_mutex_unlock(content->lock);
         qd_message_free(msg);
     }
     return 0;
@@ -1750,12 +1754,14 @@ static char *test_q2_ignore_headers(void *context)
     assert(header_ct);
     assert(!_Q2_holdoff_should_block_LH(content));
 
+    sys_mutex_lock(content->lock);
     // Now append buffers until Q2 blocks
     while (!_Q2_holdoff_should_block_LH(content)) {
         qd_buffer_t *buffy = qd_buffer();
         qd_buffer_insert(buffy, qd_buffer_capacity(buffy));
         DEQ_INSERT_TAIL(content->buffers, buffy);
     }
+    sys_mutex_unlock(content->lock);
 
     // expect: block occurs when length == QD_QLIMIT_Q2_UPPER + header_ct
     if (DEQ_SIZE(content->buffers) != QD_QLIMIT_Q2_UPPER + header_ct) {
@@ -1764,12 +1770,13 @@ static char *test_q2_ignore_headers(void *context)
     }
 
     // now remove buffers until Q2 is relieved
-
+    sys_mutex_lock(content->lock);
     while (!_Q2_holdoff_should_unblock_LH(content)) {
         qd_buffer_t *buffy = DEQ_TAIL(content->buffers);
         DEQ_REMOVE_TAIL(content->buffers);
         qd_buffer_free(buffy);
     }
+    sys_mutex_unlock(content->lock);
 
     // expect: Q2 deactivates when list length < QD_QDLIMIT_Q2_LOWER + header_ct
     if (DEQ_SIZE(content->buffers) != (QD_QLIMIT_Q2_LOWER + header_ct) - 1) {