Richard W.M. Jones
2022-Sep-28 17:25 UTC
[Libguestfs] [PATCH libnbd v3 2/6] 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. Reviewed-by: Laszlo Ersek <lersek at redhat.com> --- generator/C.ml | 59 +++++++++++++++++++++++++++++++++++-- tests/errors-connect-null.c | 4 +++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/generator/C.ml b/generator/C.ml index 66498a7d88..6e4538fd34 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,28 @@ let | UInt64 n -> [n] | UIntPtr n -> [n] +(* Map function arguments to __attribute__((nonnull)) annotations. + * Because a single arg may map to multiple C parameters this + * returns a list. Note: true => add attribute nonnull. + *) +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 ] + (* numeric and other non-pointer types are not able to 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 +238,21 @@ let let print_fndecl ?wrap ?closure_style name args optargs ret pr "extern "; print_call ?wrap ?closure_style name args optargs ret; - pr ";\n" + + (* Output __attribute__((nonnull)) for the function parameters: + * eg. struct nbd_handle *, int, char * + * => [ true, false, true ] + * => LIBNBD_ATTRIBUTE_NONNULL((1,3)) + * => __attribute__((nonnull,(1,3))) + *) + let nns : bool list + [ true ] (* struct nbd_handle * *) + @ List.flatten (List.map arg_attr_nonnull args) + @ List.flatten (List.map optarg_attr_nonnull optargs) 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 +385,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 +431,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 +819,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"; diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c index 27b21f0f8f..b975181afb 100644 --- a/tests/errors-connect-null.c +++ b/tests/errors-connect-null.c @@ -27,6 +27,10 @@ #include <string.h> #include <errno.h> +/* GCC will warn that we are passing NULL (or worse), so to do this + * test we must remove the header file attribute. + */ +#define LIBNBD_ATTRIBUTE_NONNULL(s) #include <libnbd.h> static char *progname; -- 2.37.0.rc2
Eric Blake
2022-Sep-28 21:53 UTC
[Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters
On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote:> For API parameters that are pointers and must not be NULL, add the > appropriate GCC annotations. > > Reviewed-by: Laszlo Ersek <lersek at redhat.com> > --- > generator/C.ml | 59 +++++++++++++++++++++++++++++++++++-- > tests/errors-connect-null.c | 4 +++ > 2 files changed, 61 insertions(+), 2 deletions(-)> @@ -216,7 +238,21 @@ let > let print_fndecl ?wrap ?closure_style name args optargs ret > pr "extern "; > print_call ?wrap ?closure_style name args optargs ret; > - pr ";\n" > + > + (* Output __attribute__((nonnull)) for the function parameters: > + * eg. struct nbd_handle *, int, char * > + * => [ true, false, true ] > + * => LIBNBD_ATTRIBUTE_NONNULL((1,3)) > + * => __attribute__((nonnull,(1,3)))Comment typo: No ',' before the innermost '('.> + *) > + let nns : bool list > + [ true ] (* struct nbd_handle * *) > + @ List.flatten (List.map arg_attr_nonnull args) > + @ List.flatten (List.map optarg_attr_nonnull optargs) 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) > cbargsThe typo's easy to fix, Reviewed-by: Eric Blake <eblake at redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
2022-Sep-29 13:25 UTC
[Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters
On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote:> For API parameters that are pointers and must not be NULL, add the > appropriate GCC annotations. > > Reviewed-by: Laszlo Ersek <lersek at redhat.com> > --- > generator/C.ml | 59 +++++++++++++++++++++++++++++++++++-- > tests/errors-connect-null.c | 4 +++ > 2 files changed, 61 insertions(+), 2 deletions(-)> @@ -216,7 +238,21 @@ let > let print_fndecl ?wrap ?closure_style name args optargs ret > pr "extern "; > print_call ?wrap ?closure_style name args optargs ret; > - pr ";\n" > + > + (* Output __attribute__((nonnull)) for the function parameters: > + * eg. struct nbd_handle *, int, char * > + * => [ true, false, true ] > + * => LIBNBD_ATTRIBUTE_NONNULL((1,3)) > + * => __attribute__((nonnull,(1,3)))Style question. Do we want to REQUIRE the clients of this macro to pass in (), or would it be better to have a varargs format?> + *) > + let nns : bool list > + [ true ] (* struct nbd_handle * *) > + @ List.flatten (List.map arg_attr_nonnull args) > + @ List.flatten (List.map optarg_attr_nonnull optargs) 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)For generated code, it is just as easy to cope with either style (we can strip a layer of () if we want a varargs format).> > let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true) > cbargs > @@ -349,6 +385,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";This definition is what requires us to pass in our own (). That is, our end result is going to be one of: __attribute__((__nonnull__(1) )) __attribute__((__nonnull__(1, 2) )) but the difference is whether we must pass exactly one macro argument, and where that argument must include () even when there is only one parameter to be marked (what you coded): LIBNBD_ATTRIBUTE_NONNULL((1)) LIBNBD_ATTRIBUTE_NONNULL((1, 3)) vs. ease-of-use in supplying the () as part of the macro definition itself by using #define MACRO(...) and __VA_ARGS__: LIBNBD_ATTRIBUTE_NONNULL(1) LIBNBD_ATTRIBUTE_NONNULL(1, 3) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org