Re: [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