On Tue, Sep 27, 2022 at 01:25:55PM -0500, Eric Blake wrote: > 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.
Some programmers will write it as: let nns = ... in let nns' = ... in let nns'' = ... in let nns''' = ... in where the ' character is pronounced "prime". Whether that's more or less confusing I'll leave up to you to decide :-) > > @@ -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? So firstly this pragma is necessary in order to get rid of a warning that would otherwise cause an error when using -Werror mode. It only disables the warning and GCC may still compile away the checks. I checked the asm just now and ... it does appear to be getting rid of the checks! How annoying is that? > 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. Let me try something like that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
