| Message ID | 20201007115234.1482603-1-i.maximets@ovn.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | flex-array: allow arrays of unions with flexible members. | expand |
On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: > There is a common pattern on how to allocate memory for a flexible-size > structure, e.g. > > union { > struct flex f; /* Structure that contains a flexible array. */ > char buf[MAX_SIZE]; /* Memeory buffer for structure 'flex' and > its flexible array. */ > }; > > There is another exmaple of such thing in CMSG manpage with the > alignment purposes: > > union { /* Ancillary data buffer, wrapped in a union > in order to ensure it is suitably aligned */ > char buf[CMSG_SPACE(sizeof(myfds))]; > struct cmsghdr align; > } u; > > Such unions could form an array in case user wants multiple > instances of them. For example, if you want receive up to > 32 network packets via recvmmsg(), you will need 32 unions like 'u'. > Open vSwitch does exactly that and fails the check. > > Disabling this check by default for unions. Adding new option > -Wflex-union-array to enable it back. This option works only > if -Wflex-array-array enabled (which is default behavior). > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > Not sure if this is a best way to fix the issue, but it works fine for > openvswitch project. The actual code in question that makes sparse fail > OVS build could be found here: > https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 This fixes your problem for -Wflexible-array-array but the same will happen with -Wflexible-array-sizeof (and you're using sizeof() on such flexible unions) and -Wflexible-array-nested. So, what I'm proposing is a flag to simply disable all warnings concerning flexible arrays with union types. I've also changed the flag to -Wflexible-array-union to make it clear it's closely related to -Wflexible-array-{array,nested,union}. I've also make it to be enabled by default (but I haven't made my mind on it). From 286903619edce5e723b3467e82bd6f09d4ae0031 Mon Sep 17 00:00:00 2001 From: Ilya Maximets <i.maximets@ovn.org> Date: Wed, 7 Oct 2020 13:52:34 +0200 Subject: [PATCH] flex-array: allow arrays of unions with flexible members. There is a common pattern on how to allocate memory for a flexible-size structure, e.g. union { struct flex f; /* Structure that contains a flexible array. */ char buf[MAX_SIZE]; /* Memory buffer for structure 'flex' and its flexible array. */ }; There is another example of such thing in CMSG manpage with the alignment purposes: union { /* Ancillary data buffer, wrapped in a union in order to ensure it is suitably aligned */ char buf[CMSG_SPACE(sizeof(myfds))]; struct cmsghdr align; } u; Such unions could form an array in case user wants multiple instances of them. For example, if you want receive up to 32 network packets via recvmmsg(), you will need 32 unions like 'u'. Open vSwitch does exactly that and fails the check. So, add a new option, -W[no-]flex-array-union, to enable or disable any warning concerning flexible arrays and unions. This option needs at least one of -Wflex-array-{array,nested,union} to be enabled in order to have any effect. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- options.c | 2 ++ options.h | 1 + sparse.1 | 9 +++++++++ symbol.c | 2 +- validation/flex-union-array-no.c | 9 +++++++++ validation/flex-union-array-yes.c | 11 +++++++++++ validation/flex-union-array.h | 11 +++++++++++ 7 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 validation/flex-union-array-no.c create mode 100644 validation/flex-union-array-yes.c create mode 100644 validation/flex-union-array.h diff --git a/options.c b/options.c index b46900b973a6..2736d38c0d4e 100644 --- a/options.c +++ b/options.c @@ -103,6 +103,7 @@ int Wexternal_function_has_definition = 1; int Wflexible_array_array = 1; int Wflexible_array_nested = 0; int Wflexible_array_sizeof = 0; +int Wflexible_array_union = 1; int Wimplicit_int = 1; int Winit_cstring = 0; int Wint_to_pointer_cast = 1; @@ -846,6 +847,7 @@ static const struct flag warnings[] = { { "flexible-array-array", &Wflexible_array_array }, { "flexible-array-nested", &Wflexible_array_nested }, { "flexible-array-sizeof", &Wflexible_array_sizeof }, + { "flexible-array-union", &Wflexible_array_union }, { "implicit-int", &Wimplicit_int }, { "init-cstring", &Winit_cstring }, { "int-to-pointer-cast", &Wint_to_pointer_cast }, diff --git a/options.h b/options.h index d23ed472eaac..70c6ce9bec37 100644 --- a/options.h +++ b/options.h @@ -102,6 +102,7 @@ extern int Wexternal_function_has_definition; extern int Wflexible_array_array; extern int Wflexible_array_nested; extern int Wflexible_array_sizeof; +extern int Wflexible_array_union; extern int Wimplicit_int; extern int Winit_cstring; extern int Wint_to_pointer_cast; diff --git a/sparse.1 b/sparse.1 index 9b1a59c6b9d4..ed528fd19dde 100644 --- a/sparse.1 +++ b/sparse.1 @@ -278,6 +278,15 @@ possibly recursively. Sparse does not issue these warnings by default. . .TP +.B -Wflexible-array-union +Enable the warnings regarding flexible arrays and unions. +To have any effect, at least one of \fB-Wflexible-array-array\fR, +\fB-Wflexible-array-nested\fR or \fB-Wflexible-array-sizeof\fR must also +be enabled. + +Sparse does issue these warnings by default. +. +.TP .B \-Winit\-cstring Warn about initialization of a char array with a too long constant C string. diff --git a/symbol.c b/symbol.c index 97c2e35d3570..9ae827c1d764 100644 --- a/symbol.c +++ b/symbol.c @@ -214,7 +214,7 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance if (info.flex_array) { info.has_flex_array = 1; } - if (info.has_flex_array) + if (info.has_flex_array && (!is_union_type(sym) || Wflexible_array_union)) sym->has_flex_array = 1; sym->bit_size = bit_size; return sym; diff --git a/validation/flex-union-array-no.c b/validation/flex-union-array-no.c new file mode 100644 index 000000000000..2857fd9b91a0 --- /dev/null +++ b/validation/flex-union-array-no.c @@ -0,0 +1,9 @@ +#include "flex-union-array.h" + +/* + * check-name: flex-array-union-no + * check-command: sparse -Wflexible-array-array -Wno-flexible-array-union $file + * + * check-error-start + * check-error-end + */ diff --git a/validation/flex-union-array-yes.c b/validation/flex-union-array-yes.c new file mode 100644 index 000000000000..c8aa7966c811 --- /dev/null +++ b/validation/flex-union-array-yes.c @@ -0,0 +1,11 @@ +#include "flex-union-array.h" + +/* + * check-name: flex-array-union-yes + * check-command: sparse -Wflexible-array-array -Wflexible-array-union $file + * + * check-error-start +flex-union-array-yes.c: note: in included file: +flex-union-array.h:11:17: warning: array of flexible structures + * check-error-end + */ diff --git a/validation/flex-union-array.h b/validation/flex-union-array.h new file mode 100644 index 000000000000..b2a74d1a06ea --- /dev/null +++ b/validation/flex-union-array.h @@ -0,0 +1,11 @@ +struct s_flex { + int i; + long f[]; +}; + +union s { + struct s_flex flex; + char buf[200]; +}; + +static union s a[2];
On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: > The actual code in question that makes sparse fail > OVS build could be found here: > https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 I'm impressed and surprised you're using of includes just for Sparse. I also see that this is since 2011. Just for my curiosity, have you an idea for why exactly this was needed and if it is still really needed? -- Luc
On 10/8/20 1:09 AM, Luc Van Oostenryck wrote: > On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: >> There is a common pattern on how to allocate memory for a flexible-size >> structure, e.g. >> >> union { >> struct flex f; /* Structure that contains a flexible array. */ >> char buf[MAX_SIZE]; /* Memeory buffer for structure 'flex' and >> its flexible array. */ >> }; >> >> There is another exmaple of such thing in CMSG manpage with the >> alignment purposes: >> >> union { /* Ancillary data buffer, wrapped in a union >> in order to ensure it is suitably aligned */ >> char buf[CMSG_SPACE(sizeof(myfds))]; >> struct cmsghdr align; >> } u; >> >> Such unions could form an array in case user wants multiple >> instances of them. For example, if you want receive up to >> 32 network packets via recvmmsg(), you will need 32 unions like 'u'. >> Open vSwitch does exactly that and fails the check. >> >> Disabling this check by default for unions. Adding new option >> -Wflex-union-array to enable it back. This option works only >> if -Wflex-array-array enabled (which is default behavior). >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> >> Not sure if this is a best way to fix the issue, but it works fine for >> openvswitch project. The actual code in question that makes sparse fail >> OVS build could be found here: >> https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 > > This fixes your problem for -Wflexible-array-array but the same > will happen with -Wflexible-array-sizeof (and you're using sizeof() > on such flexible unions) and -Wflexible-array-nested. I thought that it will fail some other checks too, but for some reason it doesn't. But, yes, you're right, It sounds safer to disable all of them to avoid possible issues in the future since we're actually using these unions. > > So, what I'm proposing is a flag to simply disable all warnings > concerning flexible arrays with union types. I've also changed > the flag to -Wflexible-array-union to make it clear it's closely > related to -Wflexible-array-{array,nested,union}. I've also make > it to be enabled by default (but I haven't made my mind on it). I thought about naming the option 'flexible-array-union-array' in my original patch, but that doesn't look good. Patch below looks good to me (one nitpick inline) and works fine if I'm disabling the option. I'd like it disabled by default, but I'm completely biased, so it's up to you. We have a TravisCI instance that uses sparse, so we'll need to disable this option there and update our documentation to suggest this new flag to developers. Sure, I'd like to not do that. :) > > > From 286903619edce5e723b3467e82bd6f09d4ae0031 Mon Sep 17 00:00:00 2001 > From: Ilya Maximets <i.maximets@ovn.org> > Date: Wed, 7 Oct 2020 13:52:34 +0200 > Subject: [PATCH] flex-array: allow arrays of unions with flexible members. > > There is a common pattern on how to allocate memory for a flexible-size > structure, e.g. > > union { > struct flex f; /* Structure that contains a flexible array. */ > char buf[MAX_SIZE]; /* Memory buffer for structure 'flex' and > its flexible array. */ > }; > > There is another example of such thing in CMSG manpage with the > alignment purposes: > > union { /* Ancillary data buffer, wrapped in a union > in order to ensure it is suitably aligned */ > char buf[CMSG_SPACE(sizeof(myfds))]; > struct cmsghdr align; > } u; > > Such unions could form an array in case user wants multiple > instances of them. For example, if you want receive up to > 32 network packets via recvmmsg(), you will need 32 unions like 'u'. > Open vSwitch does exactly that and fails the check. > > So, add a new option, -W[no-]flex-array-union, to enable or disable > any warning concerning flexible arrays and unions. This option needs > at least one of -Wflex-array-{array,nested,union} to be enabled in > order to have any effect. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > options.c | 2 ++ > options.h | 1 + > sparse.1 | 9 +++++++++ > symbol.c | 2 +- > validation/flex-union-array-no.c | 9 +++++++++ > validation/flex-union-array-yes.c | 11 +++++++++++ > validation/flex-union-array.h | 11 +++++++++++ > 7 files changed, 44 insertions(+), 1 deletion(-) > create mode 100644 validation/flex-union-array-no.c > create mode 100644 validation/flex-union-array-yes.c > create mode 100644 validation/flex-union-array.h Since you renamed the option, it might make sense to rename files to 'flex-array-union...'. > > diff --git a/options.c b/options.c > index b46900b973a6..2736d38c0d4e 100644 > --- a/options.c > +++ b/options.c > @@ -103,6 +103,7 @@ int Wexternal_function_has_definition = 1; > int Wflexible_array_array = 1; > int Wflexible_array_nested = 0; > int Wflexible_array_sizeof = 0; > +int Wflexible_array_union = 1; > int Wimplicit_int = 1; > int Winit_cstring = 0; > int Wint_to_pointer_cast = 1; > @@ -846,6 +847,7 @@ static const struct flag warnings[] = { > { "flexible-array-array", &Wflexible_array_array }, > { "flexible-array-nested", &Wflexible_array_nested }, > { "flexible-array-sizeof", &Wflexible_array_sizeof }, > + { "flexible-array-union", &Wflexible_array_union }, > { "implicit-int", &Wimplicit_int }, > { "init-cstring", &Winit_cstring }, > { "int-to-pointer-cast", &Wint_to_pointer_cast }, > diff --git a/options.h b/options.h > index d23ed472eaac..70c6ce9bec37 100644 > --- a/options.h > +++ b/options.h > @@ -102,6 +102,7 @@ extern int Wexternal_function_has_definition; > extern int Wflexible_array_array; > extern int Wflexible_array_nested; > extern int Wflexible_array_sizeof; > +extern int Wflexible_array_union; > extern int Wimplicit_int; > extern int Winit_cstring; > extern int Wint_to_pointer_cast; > diff --git a/sparse.1 b/sparse.1 > index 9b1a59c6b9d4..ed528fd19dde 100644 > --- a/sparse.1 > +++ b/sparse.1 > @@ -278,6 +278,15 @@ possibly recursively. > Sparse does not issue these warnings by default. > . > .TP > +.B -Wflexible-array-union > +Enable the warnings regarding flexible arrays and unions. > +To have any effect, at least one of \fB-Wflexible-array-array\fR, > +\fB-Wflexible-array-nested\fR or \fB-Wflexible-array-sizeof\fR must also > +be enabled. > + > +Sparse does issue these warnings by default. > +. > +.TP > .B \-Winit\-cstring > Warn about initialization of a char array with a too long constant C string. > > diff --git a/symbol.c b/symbol.c > index 97c2e35d3570..9ae827c1d764 100644 > --- a/symbol.c > +++ b/symbol.c > @@ -214,7 +214,7 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance > if (info.flex_array) { > info.has_flex_array = 1; > } > - if (info.has_flex_array) > + if (info.has_flex_array && (!is_union_type(sym) || Wflexible_array_union)) > sym->has_flex_array = 1; > sym->bit_size = bit_size; > return sym; > diff --git a/validation/flex-union-array-no.c b/validation/flex-union-array-no.c > new file mode 100644 > index 000000000000..2857fd9b91a0 > --- /dev/null > +++ b/validation/flex-union-array-no.c > @@ -0,0 +1,9 @@ > +#include "flex-union-array.h" > + > +/* > + * check-name: flex-array-union-no > + * check-command: sparse -Wflexible-array-array -Wno-flexible-array-union $file > + * > + * check-error-start > + * check-error-end > + */ > diff --git a/validation/flex-union-array-yes.c b/validation/flex-union-array-yes.c > new file mode 100644 > index 000000000000..c8aa7966c811 > --- /dev/null > +++ b/validation/flex-union-array-yes.c > @@ -0,0 +1,11 @@ > +#include "flex-union-array.h" > + > +/* > + * check-name: flex-array-union-yes > + * check-command: sparse -Wflexible-array-array -Wflexible-array-union $file > + * > + * check-error-start > +flex-union-array-yes.c: note: in included file: > +flex-union-array.h:11:17: warning: array of flexible structures > + * check-error-end > + */ > diff --git a/validation/flex-union-array.h b/validation/flex-union-array.h > new file mode 100644 > index 000000000000..b2a74d1a06ea > --- /dev/null > +++ b/validation/flex-union-array.h > @@ -0,0 +1,11 @@ > +struct s_flex { > + int i; > + long f[]; > +}; > + > +union s { > + struct s_flex flex; > + char buf[200]; > +}; > + > +static union s a[2]; >
On 10/8/20 1:15 AM, Luc Van Oostenryck wrote: > On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: >> The actual code in question that makes sparse fail >> OVS build could be found here: >> https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 > > I'm impressed and surprised you're using of includes just for Sparse. > I also see that this is since 2011. Just for my curiosity, have > you an idea for why exactly this was needed and if it is still > really needed? There are some Sparse-related headers that could be safely removed now because required functionality is already supported by Sparse. We need to clean this up someday, e.g. OVS builds fine without include/sparse/threads.h since Sparse 0.5.1. However, there are headers that are necessary for successful build. There are few classes of issues that these headers are targeted on: 1. Missing functionality in Sparse. For example, it doesn't know about __builtin_ia32_pause, so we have to have include/sparse/xmmintrin.h. Sparse also doesn't know __atomic_load_n that comes from some DPDK headers. OVS itself avoids using builtin atomics if __CHECKER__ defined. DPDK library also has some issues with types in __sync_add_and_fetch, but I do not remember exact problem. We have include/sparse/rte_atomic.h for it. These are from the top of my head. I could go through our specific headers and make a list of missing features someday if you're interested. 2. Sparse complains on standard libraries. Complains on PTHREAD_MUTEX_INITIALIZER: error: Using plain integer as NULL pointer So, we have to have include/sparse/pthread.h. Maybe some other examples, but I do not remember right now. 3. Issues inside external libraries. Ex. numa.h header from libnuma contains non-ANSI function declarations. So we have include/sparse/numa.h. 4. Issues with restricted types (these are heaviest). OVS uses restricted types like 'ovs_be32' inside (with __attribute__((bitwise))), but standard functions like htonl() operates with usual 'uint32_t'. While it's safe to use these exact functions, Sparse complains about type mismatch. One option here is to explicitly cast arguments with (OVS_FORCE ovs_be32) each time, but that is impractical (too many occurrences, ~4K lines of code only for hton/ntoh conversions) and will make code less readable. Much easier to override these functions just for Sparse. Ex. include/sparse/netinet/in.h. Similar issue with some data types that goes from external libraries and system headers. e.g. DPDK library operates with its own types like rte_be32_t. While it's completely safe to mix them with ovs_be32, Sparse doesn't know about that, because DPDK doesn't mark them as bitwise. This issue might be fixed on DPDK side, I guess. But Sparse will complain about different types even if these types defined in exactly same way. e.g. following test fails: diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c index 0583461c..9284bd05 100644 --- a/validation/bitwise-cast.c +++ b/validation/bitwise-cast.c @@ -35,6 +35,18 @@ static __be32 quuy(void) return (__attribute__((force)) __be32) 1730; } +/* Implicit casts of equally defined types, should be legal? */ +typedef u32 __attribute__((bitwise)) __my_be32_1; +typedef u32 __attribute__((bitwise)) __my_be32_2; + +static __my_be32_1 my_type(void) +{ + __my_be32_2 x = (__attribute__((force)) __my_be32_2) 0x2a; + + return x; +} + + /* * check-name: conversions to bitwise types * check-command: sparse -Wbitwise $file --- bitwise-cast.c:46:16: warning: incorrect type in return expression (different base types) bitwise-cast.c:46:16: expected restricted __my_be32_1 bitwise-cast.c:46:16: got restricted __my_be32_2 [usertype] x This might be not a full list of issues we have, but this is what I can remember right now. Best regards, Ilya Maximets.
On 10/8/20 11:13 AM, Ilya Maximets wrote: > On 10/8/20 1:15 AM, Luc Van Oostenryck wrote: >> On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: >>> The actual code in question that makes sparse fail >>> OVS build could be found here: >>> https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 >> >> I'm impressed and surprised you're using of includes just for Sparse. >> I also see that this is since 2011. Just for my curiosity, have >> you an idea for why exactly this was needed and if it is still >> really needed? > > There are some Sparse-related headers that could be safely removed now > because required functionality is already supported by Sparse. We need > to clean this up someday, e.g. OVS builds fine without > include/sparse/threads.h since Sparse 0.5.1. > > However, there are headers that are necessary for successful build. > There are few classes of issues that these headers are targeted on: > > 1. Missing functionality in Sparse. > For example, it doesn't know about __builtin_ia32_pause, so we have to have > include/sparse/xmmintrin.h. > Sparse also doesn't know __atomic_load_n that comes from some DPDK headers. > OVS itself avoids using builtin atomics if __CHECKER__ defined. > DPDK library also has some issues with types in __sync_add_and_fetch, but I > do not remember exact problem. We have include/sparse/rte_atomic.h for it. > These are from the top of my head. I could go through our specific headers > and make a list of missing features someday if you're interested. > > 2. Sparse complains on standard libraries. > Complains on PTHREAD_MUTEX_INITIALIZER: > error: Using plain integer as NULL pointer > So, we have to have include/sparse/pthread.h. > Maybe some other examples, but I do not remember right now. > > 3. Issues inside external libraries. > Ex. numa.h header from libnuma contains non-ANSI function declarations. > So we have include/sparse/numa.h. > > 4. Issues with restricted types (these are heaviest). > OVS uses restricted types like 'ovs_be32' inside (with __attribute__((bitwise))), > but standard functions like htonl() operates with usual 'uint32_t'. While it's > safe to use these exact functions, Sparse complains about type mismatch. One > option here is to explicitly cast arguments with (OVS_FORCE ovs_be32) each time, > but that is impractical (too many occurrences, ~4K lines of code only for hton/ntoh > conversions) and will make code less readable. Much easier to override these > functions just for Sparse. Ex. include/sparse/netinet/in.h. > > Similar issue with some data types that goes from external libraries and system > headers. e.g. DPDK library operates with its own types like rte_be32_t. While > it's completely safe to mix them with ovs_be32, Sparse doesn't know about that, > because DPDK doesn't mark them as bitwise. This issue might be fixed on DPDK > side, I guess. But Sparse will complain about different types even if these types > defined in exactly same way. e.g. following test fails: > > diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c > index 0583461c..9284bd05 100644 > --- a/validation/bitwise-cast.c > +++ b/validation/bitwise-cast.c > @@ -35,6 +35,18 @@ static __be32 quuy(void) > return (__attribute__((force)) __be32) 1730; > } > > +/* Implicit casts of equally defined types, should be legal? */ I do understand why this is illegal. Otherwise it will be not possible to create le and be types. However, since the main purpose of 'bitwise' attribute, AFAIU, is to create le and be types, maybe it make sense to have 2 different attributes? e.g. 'bitwise_le' and 'bitwise_be'. This way Sparse will be able to detect that 2 different types ovs_be32 and rte_be32_t could be safely mixed, if both defined with attribute 'bitwise_be' and has same base type uint32_t. > +typedef u32 __attribute__((bitwise)) __my_be32_1; > +typedef u32 __attribute__((bitwise)) __my_be32_2; > + > +static __my_be32_1 my_type(void) > +{ > + __my_be32_2 x = (__attribute__((force)) __my_be32_2) 0x2a; > + > + return x; > +} > + > + > /* > * check-name: conversions to bitwise types > * check-command: sparse -Wbitwise $file > --- > > bitwise-cast.c:46:16: warning: incorrect type in return expression (different base types) > bitwise-cast.c:46:16: expected restricted __my_be32_1 > bitwise-cast.c:46:16: got restricted __my_be32_2 [usertype] x > > > This might be not a full list of issues we have, but this is what I can remember right now. > > Best regards, Ilya Maximets. >
On Thu, Oct 08, 2020 at 08:36:02AM +0200, Ilya Maximets wrote: > On 10/8/20 1:09 AM, Luc Van Oostenryck wrote: > > On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: > >> There is a common pattern on how to allocate memory for a flexible-size > >> structure, e.g. > >> > >> union { > >> struct flex f; /* Structure that contains a flexible array. */ > >> char buf[MAX_SIZE]; /* Memeory buffer for structure 'flex' and > >> its flexible array. */ > >> }; > >> > >> There is another exmaple of such thing in CMSG manpage with the > >> alignment purposes: > >> > >> union { /* Ancillary data buffer, wrapped in a union > >> in order to ensure it is suitably aligned */ > >> char buf[CMSG_SPACE(sizeof(myfds))]; > >> struct cmsghdr align; > >> } u; > >> > >> Such unions could form an array in case user wants multiple > >> instances of them. For example, if you want receive up to > >> 32 network packets via recvmmsg(), you will need 32 unions like 'u'. > >> Open vSwitch does exactly that and fails the check. > >> > >> Disabling this check by default for unions. Adding new option > >> -Wflex-union-array to enable it back. This option works only > >> if -Wflex-array-array enabled (which is default behavior). > >> > >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > >> --- > >> > >> Not sure if this is a best way to fix the issue, but it works fine for > >> openvswitch project. The actual code in question that makes sparse fail > >> OVS build could be found here: > >> https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 > > > > This fixes your problem for -Wflexible-array-array but the same > > will happen with -Wflexible-array-sizeof (and you're using sizeof() > > on such flexible unions) and -Wflexible-array-nested. > > I thought that it will fail some other checks too, but for some reason > it doesn't. But, yes, you're right, It sounds safer to disable all > of them to avoid possible issues in the future since we're actually > using these unions. Thanks for giving it a try. > > options.c | 2 ++ > > options.h | 1 + > > sparse.1 | 9 +++++++++ > > symbol.c | 2 +- > > validation/flex-union-array-no.c | 9 +++++++++ > > validation/flex-union-array-yes.c | 11 +++++++++++ > > validation/flex-union-array.h | 11 +++++++++++ > > 7 files changed, 44 insertions(+), 1 deletion(-) > > create mode 100644 validation/flex-union-array-no.c > > create mode 100644 validation/flex-union-array-yes.c > > create mode 100644 validation/flex-union-array.h > > Since you renamed the option, it might make sense to rename > files to 'flex-array-union...'. Well yes, I left them more or less on purpose but renamed them now. -- Luc
On Thu, Oct 08, 2020 at 12:04:55PM +0200, Ilya Maximets wrote: > On 10/8/20 11:13 AM, Ilya Maximets wrote: > > On 10/8/20 1:15 AM, Luc Van Oostenryck wrote: > >> On Wed, Oct 07, 2020 at 01:52:34PM +0200, Ilya Maximets wrote: > >>> The actual code in question that makes sparse fail > >>> OVS build could be found here: > >>> https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 > >> > >> I'm impressed and surprised you're using of includes just for Sparse. > >> I also see that this is since 2011. Just for my curiosity, have > >> you an idea for why exactly this was needed and if it is still > >> really needed? > > > > There are some Sparse-related headers that could be safely removed now > > because required functionality is already supported by Sparse. We need > > to clean this up someday, e.g. OVS builds fine without > > include/sparse/threads.h since Sparse 0.5.1. > > > > However, there are headers that are necessary for successful build. > > There are few classes of issues that these headers are targeted on: > > > > 1. Missing functionality in Sparse. > > For example, it doesn't know about __builtin_ia32_pause, so we have to have > > include/sparse/xmmintrin.h. > > Sparse also doesn't know __atomic_load_n that comes from some DPDK headers. > > OVS itself avoids using builtin atomics if __CHECKER__ defined. > > DPDK library also has some issues with types in __sync_add_and_fetch, but I > > do not remember exact problem. We have include/sparse/rte_atomic.h for it. > > These are from the top of my head. I could go through our specific headers > > and make a list of missing features someday if you're interested. OK, I see. I'll add __builtin_ia32_pause() now and I'll look at __atomic_load_n(), __sync_add_and_fetch() and friends soon. > > 2. Sparse complains on standard libraries. > > Complains on PTHREAD_MUTEX_INITIALIZER: > > error: Using plain integer as NULL pointer > > So, we have to have include/sparse/pthread.h. > > Maybe some other examples, but I do not remember right now. > > > > 3. Issues inside external libraries. > > Ex. numa.h header from libnuma contains non-ANSI function declarations. > > So we have include/sparse/numa.h. It's always possible to use -Wno-non-pointer-null and -Wno-old_-style-definition but ... > > 4. Issues with restricted types (these are heaviest). > > OVS uses restricted types like 'ovs_be32' inside (with __attribute__((bitwise))), > > but standard functions like htonl() operates with usual 'uint32_t'. While it's > > safe to use these exact functions, Sparse complains about type mismatch. One > > option here is to explicitly cast arguments with (OVS_FORCE ovs_be32) each time, > > but that is impractical (too many occurrences, ~4K lines of code only for hton/ntoh > > conversions) and will make code less readable. Much easier to override these > > functions just for Sparse. Ex. include/sparse/netinet/in.h. > > > > Similar issue with some data types that goes from external libraries and system > > headers. e.g. DPDK library operates with its own types like rte_be32_t. While > > it's completely safe to mix them with ovs_be32, Sparse doesn't know about that, > > because DPDK doesn't mark them as bitwise. This issue might be fixed on DPDK > > side, I guess. But Sparse will complain about different types even if these types > > defined in exactly same way. e.g. following test fails: > > > > diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c > > index 0583461c..9284bd05 100644 > > --- a/validation/bitwise-cast.c > > +++ b/validation/bitwise-cast.c > > @@ -35,6 +35,18 @@ static __be32 quuy(void) > > return (__attribute__((force)) __be32) 1730; > > } > > > > +/* Implicit casts of equally defined types, should be legal? */ > > I do understand why this is illegal. Otherwise it will be not possible to > create le and be types. However, since the main purpose of 'bitwise' attribute, > AFAIU, is to create le and be types, maybe it make sense to have 2 different > attributes? e.g. 'bitwise_le' and 'bitwise_be'. This way Sparse will be able > to detect that 2 different types ovs_be32 and rte_be32_t could be safely mixed, > if both defined with attribute 'bitwise_be' and has same base type uint32_t. Well, one of the key characteristic of __bitwise is that it creates new, distinct, incompatible types and it is very much used as such. For example, in the kernel the 6 {be,le}{16,32,64} are maybe the most used but there are about 100 other bitwise types that are defined (like poll_t or pci_power_t) just to have a distinct type. So, for the problem with rte_be32_t & ovs_be32, the solutions should be to have a common definition, either via a macro or a typedef. Not an easy thing with external libraries. Best regards, -- Luc
diff --git a/options.c b/options.c index b46900b9..cb935832 100644 --- a/options.c +++ b/options.c @@ -103,6 +103,7 @@ int Wexternal_function_has_definition = 1; int Wflexible_array_array = 1; int Wflexible_array_nested = 0; int Wflexible_array_sizeof = 0; +int Wflexible_union_array = 0; int Wimplicit_int = 1; int Winit_cstring = 0; int Wint_to_pointer_cast = 1; @@ -846,6 +847,7 @@ static const struct flag warnings[] = { { "flexible-array-array", &Wflexible_array_array }, { "flexible-array-nested", &Wflexible_array_nested }, { "flexible-array-sizeof", &Wflexible_array_sizeof }, + { "flexible-union-array", &Wflexible_union_array }, { "implicit-int", &Wimplicit_int }, { "init-cstring", &Winit_cstring }, { "int-to-pointer-cast", &Wint_to_pointer_cast }, diff --git a/options.h b/options.h index d23ed472..a20c9f61 100644 --- a/options.h +++ b/options.h @@ -102,6 +102,7 @@ extern int Wexternal_function_has_definition; extern int Wflexible_array_array; extern int Wflexible_array_nested; extern int Wflexible_array_sizeof; +extern int Wflexible_union_array; extern int Wimplicit_int; extern int Winit_cstring; extern int Wint_to_pointer_cast; diff --git a/sparse.1 b/sparse.1 index 9b1a59c6..ac45b67c 100644 --- a/sparse.1 +++ b/sparse.1 @@ -278,6 +278,13 @@ possibly recursively. Sparse does not issue these warnings by default. . .TP +.B -Wflexible-union-array +Warn about arrays of unions containing a flexible array. + +Sparse does not issue these warnings by default. +To turn them on, \fB-Wflexible-array-array\fR should be enabled too. +. +.TP .B \-Winit\-cstring Warn about initialization of a char array with a too long constant C string. diff --git a/symbol.c b/symbol.c index 97c2e35d..7ad7351b 100644 --- a/symbol.c +++ b/symbol.c @@ -271,7 +271,8 @@ static struct symbol * examine_array_type(struct symbol *sym) bit_size = -1; } } - if (has_flexible_array(base_type) && Wflexible_array_array) + if (has_flexible_array(base_type) && Wflexible_array_array && + (!is_union_type(base_type) || Wflexible_union_array)) warning(sym->pos, "array of flexible structures"); alignment = base_type->ctype.alignment; if (!sym->ctype.alignment) diff --git a/symbol.h b/symbol.h index e1f53926..b208ce87 100644 --- a/symbol.h +++ b/symbol.h @@ -425,6 +425,13 @@ static inline int is_array_type(struct symbol *type) return type->type == SYM_ARRAY; } +static inline int is_union_type(struct symbol *type) +{ + if (type->type == SYM_NODE) + type = type->ctype.base_type; + return type->type == SYM_UNION; +} + static inline int is_float_type(struct symbol *type) { if (type->type == SYM_NODE) diff --git a/validation/flex-union-array-no.c b/validation/flex-union-array-no.c new file mode 100644 index 00000000..d370971c --- /dev/null +++ b/validation/flex-union-array-no.c @@ -0,0 +1,9 @@ +#include "flex-union-array.h" + +/* + * check-name: flex-union-array-no + * check-command: sparse -Wflexible-array-array -Wno-flexible-union-array $file + * + * check-error-start + * check-error-end + */ diff --git a/validation/flex-union-array-yes.c b/validation/flex-union-array-yes.c new file mode 100644 index 00000000..4c594386 --- /dev/null +++ b/validation/flex-union-array-yes.c @@ -0,0 +1,11 @@ +#include "flex-union-array.h" + +/* + * check-name: flex-union-array-yes + * check-command: sparse -Wflexible-array-array -Wflexible-union-array $file + * + * check-error-start +flex-union-array-yes.c: note: in included file: +flex-union-array.h:11:17: warning: array of flexible structures + * check-error-end + */ diff --git a/validation/flex-union-array.h b/validation/flex-union-array.h new file mode 100644 index 00000000..b2a74d1a --- /dev/null +++ b/validation/flex-union-array.h @@ -0,0 +1,11 @@ +struct s_flex { + int i; + long f[]; +}; + +union s { + struct s_flex flex; + char buf[200]; +}; + +static union s a[2];
There is a common pattern on how to allocate memory for a flexible-size structure, e.g. union { struct flex f; /* Structure that contains a flexible array. */ char buf[MAX_SIZE]; /* Memeory buffer for structure 'flex' and its flexible array. */ }; There is another exmaple of such thing in CMSG manpage with the alignment purposes: union { /* Ancillary data buffer, wrapped in a union in order to ensure it is suitably aligned */ char buf[CMSG_SPACE(sizeof(myfds))]; struct cmsghdr align; } u; Such unions could form an array in case user wants multiple instances of them. For example, if you want receive up to 32 network packets via recvmmsg(), you will need 32 unions like 'u'. Open vSwitch does exactly that and fails the check. Disabling this check by default for unions. Adding new option -Wflex-union-array to enable it back. This option works only if -Wflex-array-array enabled (which is default behavior). Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Not sure if this is a best way to fix the issue, but it works fine for openvswitch project. The actual code in question that makes sparse fail OVS build could be found here: https://github.com/openvswitch/ovs/blob/39fbd2c3f0392811689ec780f09baf90faceb877/lib/netdev-linux.c#L1238 options.c | 2 ++ options.h | 1 + sparse.1 | 7 +++++++ symbol.c | 3 ++- symbol.h | 7 +++++++ validation/flex-union-array-no.c | 9 +++++++++ validation/flex-union-array-yes.c | 11 +++++++++++ validation/flex-union-array.h | 11 +++++++++++ 8 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 validation/flex-union-array-no.c create mode 100644 validation/flex-union-array-yes.c create mode 100644 validation/flex-union-array.h