thr3ads.net - Libguestfs - [Libguestfs] [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters [Sep 2022]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2022-Sep-27 14:46 UTC

[Libguestfs] [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters

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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 013f81edf4..4f758e526f 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 *)
+  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
+  | Int _ | Int64 _ | SizeT _
+  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
+
+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)
 
 let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
           cbargs @@ -349,6 +379,17 @@ 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 "#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 "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -382,7 +423,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";
@@ -773,6 +814,13 @@ let
   pr "#include \"libnbd.h\"\n";
   pr "#include \"internal.h\"\n";
   pr "\n";
+  pr "/* We check that some string parameters declared as nonnull
are\n";
+  pr " * not NULL.  This is intentional because we do not know if
the\n";
+  pr " * calling compiler checked the attributes.  So ignore
those\n";
+  pr " * warnings here.\n";
+  pr " */\n";
+  pr "#pragma GCC diagnostic ignored
\"-Wnonnull-compare\"\n";
+  pr "\n";
   List.iter print_wrapper handle_calls
 
 (* We generate a fragment of Makefile.am containing the list
-- 
2.37.0.rc2

Eric Blake

2022-Sep-27 18:25 UTC

head link

[Libguestfs] [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters

On Tue, Sep 27, 2022 at 03:46:19PM +0100, 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
ACK 1 and 2 regardless of the rest of the series.

For this one...
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index 013f81edf4..4f758e526f 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 *)
> +  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
> +  | Int _ | Int64 _ | SizeT _
> +  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
It's a little bit odd to see UIntPtr called a 'non-pointer type' -
but
technically, it is an integer rather than a pointer.  And the clincher
is that even if it represents a pointer, for our purposes it is an
opaque type, and the user can indeed pass in NULL (cast to uintptr_t)
and we should not complain.  So nothing wrong here other than maybe a
confusing comment, although I don't have wording suggestions to help.
> @@ -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)
>
I'm still getting used to OCaml's ability to rebind a variable as many
iterations as we want, even with different typing!  But this makes
sense.
> @@ -773,6 +814,13 @@ let
>    pr "#include \"libnbd.h\"\n";
>    pr "#include \"internal.h\"\n";
>    pr "\n";
> +  pr "/* We check that some string parameters declared as nonnull
are\n";
> +  pr " * not NULL.  This is intentional because we do not know if
the\n";
> +  pr " * calling compiler checked the attributes.  So ignore
those\n";
> +  pr " * warnings here.\n";
> +  pr " */\n";
> +  pr "#pragma GCC diagnostic ignored
\"-Wnonnull-compare\"\n";
Does disabling the warning actually force the compiler to emit the
nonnull check, or can it still be optimized away in spite of us
silencing the warning?  Maybe we better off writing it so that for
_this_ .c file, we pre-define LIBNBD_ATTRIBUTE_NONNULL() to be a no-op
regardless of what the included .h files say.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Libguestfs - Sep 2022 - [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Libguestfs] [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Libguestfs] [PATCH libnbd 3/5] generator: Add attribute((nonnull)) annotations to non-NULL parameters