[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

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

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

-- 
2.38.1

[PATCH 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 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 from
https://github.com/bonzini/vrc:

    # 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
    # The comma is an "AND".  The "path" here consists of a single node
    (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
    # regular expression search
    (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     | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 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..5f5ab8136a3a 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -28,6 +28,27 @@
  * These functions are re-entrant and may be used outside the global mutex.
  */
 
+/**
+ * Mark a function that can suspend when executed in coroutine context,
+ * but can handle running in non-coroutine context too.
+ *
+ * Functions that execute in coroutine context cannot be called directly from
+ * normal functions.  In the future it would be nice to enable compiler or
+ * static checker support for catching such errors.  This annotation might make
+ * it possible and in the meantime it serves as documentation.
+ *
+ * For example:
+ *
+ *   static void coroutine_fn foo(void) {
+ *       ....
+ *   }
+ */
+#ifdef __clang__
+#define coroutine_mixed_fn __attribute__((__annotate__("coroutine_mixed_fn")))
+#else
+#define coroutine_mixed_fn
+#endif
+
 /**
  * Mark a function that executes in coroutine context
  *
@@ -48,6 +69,18 @@
 #define coroutine_fn
 #endif
 
+/**
+ * Mark a function that should never be called from a coroutine context
+ *
+ * This typically means that there is an analogous, coroutine_fn function that
+ * should be used instead.
+ */
+#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 2/2] block: Add no_coroutine_fn and coroutine_mixed_fn marker

[Kevin Wolf]

Am 15.12.2022 um 18:44 hat Paolo Bonzini geschrieben:
> 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 from
> https://github.com/bonzini/vrc:
> 
>     # 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
>     # The comma is an "AND".  The "path" here consists of a single node
>     (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
>     # regular expression search
>     (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     | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 40 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..5f5ab8136a3a 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -28,6 +28,27 @@
>   * These functions are re-entrant and may be used outside the global mutex.
>   */
>  
> +/**
> + * Mark a function that can suspend when executed in coroutine context,
> + * but can handle running in non-coroutine context too.
> + *
> + * Functions that execute in coroutine context cannot be called directly from
> + * normal functions.  In the future it would be nice to enable compiler or
> + * static checker support for catching such errors.  This annotation might make
> + * it possible and in the meantime it serves as documentation.
> + *
> + * For example:
> + *
> + *   static void coroutine_fn foo(void) {

s/coroutine_fn/coroutine_mixed_fn/

> + *       ....
> + *   }
> + */
> +#ifdef __clang__
> +#define coroutine_mixed_fn __attribute__((__annotate__("coroutine_mixed_fn")))
> +#else
> +#define coroutine_mixed_fn
> +#endif
> +
>  /**
>   * Mark a function that executes in coroutine context
>   *
> @@ -48,6 +69,18 @@
>  #define coroutine_fn
>  #endif
>  
> +/**
> + * Mark a function that should never be called from a coroutine context

Maybe this could be phrased better, because coroutine_mixed_fn are
functions that are specifically meant to be called from ambiguous
context, which of course includes coroutine context.

Something like "...when the caller knows that it is in coroutine
context"?

> + * This typically means that there is an analogous, coroutine_fn function that
> + * should be used instead.
> + */
> +#ifdef __clang__
> +#define no_coroutine_fn __attribute__((__annotate__("no_coroutine_fn")))
> +#else
> +#define no_coroutine_fn
> +#endif
> +

Kevin