[PATCH] sparse: only warn about directly nested flexible arrays

[PATCH] sparse: only warn about directly nested flexible arrays

[Jacob Keller]

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(-)

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);
 }
 
 /*
-- 
2.34.1.182.ge773545c7fe7

Re: [PATCH] sparse: only warn about directly nested flexible arrays

[Luc Van Oostenryck]

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