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
Richard W.M. Jones
2022-Sep-29 13:42 UTC
[Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters
On Thu, Sep 29, 2022 at 08:25:29AM -0500, Eric Blake wrote:> 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)I'm not sure I understand - what does the second definition look like? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit