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@redhat.com> Laszlo