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.