| Message ID | 20220111233959.2301120-1-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | sparse: only warn about directly nested flexible arrays | expand |
On Tue, Jan 11, 2022 at 03:39:59PM -0800, Jacob Keller wrote: Hi, Sorry for this long delay. > Commit 02ed28909f3b ("flex-array: warn on flexible array in nested > aggregate types") Introduced a new -Wflexible-array-nested warning which > produces a warning when code nests a flexible array aggregate type > within another aggregate type. > > This can produce warnings that are difficult to understand if this > becomes nested. Consider the following example code: > > struct a { > int i; > long f[]; > }; > > struct b { > struct a a; > }; > > struct c { > struct b b; > }; > > This will produce a warning both at struct b which directly embeds the > struct a, and also at c which happens to include struct a recursively. > > It does not make sense to warn at the site of struct c. We already > produce a warning at struct b! This just confuses users and can produce > lots of extra warnings. Consider if struct b was part of some header > and all of its users now see warnings for their usages like 'struct c' > which now look like their fault, when the fault really lies with the > definition of struct b. > > To avoid this, stop proliferating has_flexible_array so that the outer > struct no longer produces a warning. I fully agree with the feeling here but your patch would also disable a warning for: struct s_flex { int i; long f[]; }; union s { struct s_flex flex; char buf[200]; }; static union s a[2]; and catching arrays containing some flexible-array-member was one of reasons to add the warning. > This patch is a response to my query at > https://lore.kernel.org/linux-sparse/64238376-3882-2449-7758-e948cb4a6e1c@intel.com/T/#u I don't agree with everything you wrote there. For example, something like "zero-sized flexible member" is meaningless to me (at least from a type system PoV, which is what Sparse is all about) because a FAM has no (static) size. It's just that struct tty_bufhead::sentinel.data will not (and must not!) be used, but this won't be checkable. > I think that proliferating this warning is unnecessary (since it will > produce one warning at the exact point where we embed the structure already) > and avoids confusing users of a header into thinking their code is at fault > when its the code in the header at fault. Yes, I fully agree. I'll see in the coming days what can be done. -- Luc
diff --git a/symbol.c b/symbol.c index 91352a3a447b..d066515fc8ed 100644 --- a/symbol.c +++ b/symbol.c @@ -231,8 +231,6 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance info.max_align = member->ctype.alignment; } - if (has_flexible_array(member)) - info.has_flex_array = 1; if (has_flexible_array(member) && Wflexible_array_nested) warning(member->pos, "nested flexible array"); fn(member, &info); diff --git a/validation/flex-array-nested.c b/validation/flex-array-nested.c index 094de2fbc392..81384ec6fd32 100644 --- a/validation/flex-array-nested.c +++ b/validation/flex-array-nested.c @@ -11,11 +11,16 @@ union u { struct f f; }; +struct v { + struct s s; +}; + // trigger the examination of the offending types -static int foo(struct s *s, union u *u) +static int foo(struct s *s, union u *u, struct v *v) { return __builtin_offsetof(typeof(*s), f) - + __builtin_offsetof(typeof(*u), f); + + __builtin_offsetof(typeof(*u), f) + + __builtin_offsetof(typeof(*v), s); } /*
Commit 02ed28909f3b ("flex-array: warn on flexible array in nested aggregate types") Introduced a new -Wflexible-array-nested warning which produces a warning when code nests a flexible array aggregate type within another aggregate type. This can produce warnings that are difficult to understand if this becomes nested. Consider the following example code: struct a { int i; long f[]; }; struct b { struct a a; }; struct c { struct b b; }; This will produce a warning both at struct b which directly embeds the struct a, and also at c which happens to include struct a recursively. It does not make sense to warn at the site of struct c. We already produce a warning at struct b! This just confuses users and can produce lots of extra warnings. Consider if struct b was part of some header and all of its users now see warnings for their usages like 'struct c' which now look like their fault, when the fault really lies with the definition of struct b. To avoid this, stop proliferating has_flexible_array so that the outer struct no longer produces a warning. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- This patch is a response to my query at https://lore.kernel.org/linux-sparse/64238376-3882-2449-7758-e948cb4a6e1c@intel.com/T/#u I think that proliferating this warning is unnecessary (since it will produce one warning at the exact point where we embed the structure already) and avoids confusing users of a header into thinking their code is at fault when its the code in the header at fault. symbol.c | 2 -- validation/flex-array-nested.c | 9 +++++++-- 2 files changed, 7 insertions(+), 4 deletions(-)