[Libguestfs] [PATCH libnbd v2 1/4] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Laszlo Ersek]

On 09/28/22 11:30, Richard W.M. Jones wrote:
> For API parameters that are pointers and must not be NULL, add the
> appropriate GCC annotations.  These are only enabled in very recent
> GCC (>= 12) because we have concerns with earlier versions, see for
> example: https://bugzilla.redhat.com/show_bug.cgi?id=1041336
> ---
>  generator/C.ml | 53 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)

This patch looks good to me, but a whole lot more documentation would
have been necessary -- the idea should be summarized in the commit
message, or in code comments:

> 
> diff --git a/generator/C.ml b/generator/C.ml
> index 66498a7d88..cafc5590e2 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -107,6 +107,26 @@ let
>    | UInt64 n -> [n]
>    | UIntPtr n -> [n]
>  
> +let arg_attr_nonnull =
> +  function
> +  (* BytesIn/Out are passed using a non-null pointer, and size_t *)
> +  | BytesIn _
> +  | BytesOut _
> +  | BytesPersistIn _
> +  | BytesPersistOut _ -> [ true; false ]
> +  (* sockaddr is also non-null pointer, and length *)
> +  | SockAddrAndLen (n, len) -> [ true; false ]
> +  (* strings should be marked as non-null *)
> +  | Path _ | String _ -> [ true ]
> +  (* list of strings should be marked as non-null *)
> +  | StringList n -> [ true ]
> +  (* other non-pointer types can never be null *)

side comment: I propose "are unable to be null".

"Cannot" is ambiguous between "must not" and "incapable of" (or "unable
to"). Usually the context resolves this, but here it doesn't.

> +  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
> +  | Int _ | Int64 _ | SizeT _
> +  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
> +

IMO it is not trivial at all why we're outputting lists here. Once I
read through the entire patch and then re-read it from the start, I
understood the idea (i.e., at second reading). That's precisely what the
commit message should prepare the reviewer for.

The idea is that a particular API parameter may map to an indivisible
sequence of C-language parameters, and only a subset of these C-level
parameters need to (or can) be marked as nonnull. Thus, the above
mapping deals with these indivisible sequences, and then:

> +let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ]
> +
>  let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
>            ?closure_style args optargs =
>    if parens then pr "(";
> @@ -216,7 +236,17 @@ let
>  let print_fndecl ?wrap ?closure_style name args optargs ret =
>    pr "extern ";
>    print_call ?wrap ?closure_style name args optargs ret;
> -  pr ";\n"
> +
> +  (* Non-null attribute. *)
> +  let nns =
> +    [ [ true ] ] (* for struct nbd_handle pointer *)
> +    @ List.map arg_attr_nonnull args
> +    @ List.map optarg_attr_nonnull optargs in
> +  let nns : bool list = List.flatten nns in
> +  let nns = List.mapi (fun i b -> (i+1, b)) nns in
> +  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
> +  let nns : string list = List.map string_of_int nns in
> +  pr "\n    LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns)
>  

here we collect the marked parameters from a *list* of such indivisible
sequences, globally counting upwards from 1, so that we can ultimately
dump the list of the flat sequence numbers of the affected C-level
parameters into a single __attribute__((__nonnull__ ....)) construct.

I really shouldn't have to re-compose the goal from the bottom up, when
reviewing a patch :/ OCaml is hugely expressive, a lot can be
accomplished in a few lines of code.

>  let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
>            cbargs =
> @@ -349,6 +379,19 @@ let
>    pr "extern \"C\" {\n";
>    pr "#endif\n";
>    pr "\n";
> +  pr "#if defined(__GNUC__)\n";
> +  pr "#define LIBNBD_GCC_VERSION \\\n";
> +  pr "    (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)\n";
> +  pr "#endif\n";
> +  pr "\n";
> +  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
> +  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /* gcc >= 12.0 */\n";
> +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ s))\n";
> +  pr "#else\n";
> +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
> +  pr "#endif\n";
> +  pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
> +  pr "\n";
>    pr "struct nbd_handle;\n";
>    pr "\n";
>    List.iter (
> @@ -382,7 +425,7 @@ let
>    pr "extern struct nbd_handle *nbd_create (void);\n";
>    pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
>    pr "\n";
> -  pr "extern void nbd_close (struct nbd_handle *h);\n";
> +  pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL */\n";
>    pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
>    pr "\n";
>    pr "extern const char *nbd_get_error (void);\n";
> @@ -770,6 +813,12 @@ let
>    pr "\n";
>    pr "#include <pthread.h>\n";
>    pr "\n";
> +  pr "/* GCC will remove NULL checks from this file for any parameter\n";
> +  pr " * annotated with attribute((nonnull)).  See RHBZ#1041336.  To\n";
> +  pr " * avoid this, disable the attribute when including libnbd.h.\n";
> +  pr " */\n";
> +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
> +  pr "\n";
>    pr "#include \"libnbd.h\"\n";
>    pr "#include \"internal.h\"\n";
>    pr "\n";
> 

The code is absolutely great; please be more considerate & gentle with
your reviewers.

With some comments added:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Laszlo