[PATCH 0/2] Make coroutine annotations ready for static analysis

[PATCH 0/2] Make coroutine annotations ready for static analysis

[Paolo Bonzini]

Clang has a generic __annotate__ attribute that can be used by
static analyzers to understand properties of functions and
analyze the control flow.

Unlike TSA annotations, the __annotate__ attribute applies to function
pointers as well, which is very fortunate because many BlockDriver
function driver run in coroutines.

Paolo

v1->v2: improved comments for patch 2

Alberto Faria (2):
  block: Add no_coroutine_fn and coroutine_mixed_fn marker
  coroutine: annotate coroutine_fn for libclang

 include/block/block-common.h | 11 +++++----
 include/qemu/coroutine.h     | 43 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

-- 
2.38.1

[PATCH v2 1/2] coroutine: annotate coroutine_fn for libclang

[Paolo Bonzini]

From: Alberto Faria <afaria@redhat.com>

Clang has a generic __annotate__ attribute that can be used by
static analyzers to understand properties of functions and
analyze the control flow.  Furthermore, unlike TSA annotations, the
__annotate__ attribute applies to function pointers as well.

As a first step towards static analysis of coroutine_fn markers,
attach the attribute to the marker when compiling with clang.

Signed-off-by: Alberto Faria <afaria@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/coroutine.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 89650a2d7fab..b0c97f6fb7ad 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -42,7 +42,11 @@
  *       ....
  *   }
  */
+#ifdef __clang__
+#define coroutine_fn __attribute__((__annotate__("coroutine_fn")))
+#else
 #define coroutine_fn
+#endif
 
 typedef struct Coroutine Coroutine;
 
-- 
2.38.1

[PATCH v2 2/2] block: Add no_coroutine_fn and coroutine_mixed_fn marker

[Paolo Bonzini]

From: Alberto Faria <afaria@redhat.com>

Add more annotations to functions, describing valid and invalid
calls from coroutine to non-coroutine context.

When applied to a function, no_coroutine_fn advertises that it should
not be called from coroutine_fn functions.  This can be because the
function blocks or, in the case of generated_co_wrapper, to enforce
that coroutine_fn functions directly call the coroutine_fn that backs
the generated_co_wrapper.

coroutine_mixed_fn instead is for function that can be called in
both coroutine and non-coroutine context, but will suspend when
called in coroutine context.  Annotating them is a first step
towards enforcing that non-annotated functions are absolutely
not going to suspend.

These can be used for example with the vrc tool:

    # find functions that *really* cannot be called from no_coroutine_fn
    (vrc) load --loader clang libblock.fa.p/meson-generated_.._block_block-gen.c.o
    (vrc) paths [no_coroutine_fn,!coroutine_mixed_fn]
    bdrv_remove_persistent_dirty_bitmap
    bdrv_create
    bdrv_can_store_new_dirty_bitmap

    # find how coroutine_fns end up calling a mixed function
    (vrc) load --loader clang --force libblock.fa.p/*.c.o
    (vrc) paths [coroutine_fn] [!no_coroutine_fn]* [coroutine_mixed_fn]
    ...
    bdrv_pread <- vhdx_log_write <- vhdx_log_write_and_flush <- vhdx_co_writev
    ...

Signed-off-by: Alberto Faria <afaria@redhat.com>
[Rebase, add coroutine_mixed_fn. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/block-common.h | 11 ++++++----
 include/qemu/coroutine.h     | 39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/include/block/block-common.h b/include/block/block-common.h
index 4749c46a5e7e..cce79bd00135 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -50,11 +50,14 @@
  * - co_wrapper_mixed_bdrv_rdlock are co_wrapper_mixed functions but
  *   automatically take and release the graph rdlock when creating a new
  *   coroutine.
+ *
+ * These functions should not be called from a coroutine_fn; instead,
+ * call the wrapped function directly.
  */
-#define co_wrapper
-#define co_wrapper_mixed
-#define co_wrapper_bdrv_rdlock
-#define co_wrapper_mixed_bdrv_rdlock
+#define co_wrapper                     no_coroutine_fn
+#define co_wrapper_mixed               no_coroutine_fn coroutine_mixed_fn
+#define co_wrapper_bdrv_rdlock         no_coroutine_fn
+#define co_wrapper_mixed_bdrv_rdlock   no_coroutine_fn coroutine_mixed_fn
 
 #include "block/dirty-bitmap.h"
 #include "block/blockjob.h"
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index b0c97f6fb7ad..3ab086aee158 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -48,6 +48,45 @@
 #define coroutine_fn
 #endif
 
+/**
+ * Mark a function that can suspend when executed in coroutine context,
+ * but can handle running in non-coroutine context too.
+ */
+#ifdef __clang__
+#define coroutine_mixed_fn __attribute__((__annotate__("coroutine_mixed_fn")))
+#else
+#define coroutine_mixed_fn
+#endif
+
+/**
+ * Mark a function that should not be called from a coroutine context.
+ * Usually there will be an analogous, coroutine_fn function that should
+ * be used instead.
+ *
+ * When the function is also marked as coroutine_mixed_fn, the function should
+ * only be called if the caller does not know whether it is in coroutine
+ * context.
+ *
+ * Functions that are only no_coroutine_fn, on the other hand, should not
+ * be called from within coroutines at all.  This for example includes
+ * functions that block.
+ *
+ * In the future it would be nice to enable compiler or static checker
+ * support for catching such errors.  This annotation is the first step
+ * towards this, and in the meantime it serves as documentation.
+ *
+ * For example:
+ *
+ *   static void no_coroutine_fn foo(void) {
+ *       ....
+ *   }
+ */
+#ifdef __clang__
+#define no_coroutine_fn __attribute__((__annotate__("no_coroutine_fn")))
+#else
+#define no_coroutine_fn
+#endif
+
 typedef struct Coroutine Coroutine;
 
 /**
-- 
2.38.1

Re: [PATCH 0/2] Make coroutine annotations ready for static analysis

[Kevin Wolf]

Am 16.12.2022 um 12:07 hat Paolo Bonzini geschrieben:
> Clang has a generic __annotate__ attribute that can be used by
> static analyzers to understand properties of functions and
> analyze the control flow.
> 
> Unlike TSA annotations, the __annotate__ attribute applies to function
> pointers as well, which is very fortunate because many BlockDriver
> function driver run in coroutines.
> 
> Paolo
> 
> v1->v2: improved comments for patch 2

Thanks, applied to the block branch.

Kevin