Re: [PATCH] tree: Fix up component_ref_sam_type handling of arrays of 0 sized elements [PR109215]

[Richard Biener <rguenther@suse.de>]

On Tue, 21 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> Our documentation sadly talks about elt_type arr[0]; as zero-length arrays,
> not arrays with zero elements.  Unfortunately, those aren't the only arrays
> which can have zero size, the same size can be also result of zero-length
> element, like in GNU C struct whatever {} or in GNU C/C++ if the element
> type is [0] array or combination thereof (dunno if Ada doesn't allow
> something similar too).  One can't do much with them, taking address of
> their elements, (no-op) copying of the elements in and out.  But they
> behave differently from arr[0] arrays e.g. in that using non-zero indexes
> in them (as long as they are within bounds as for normal arrays) is valid.
> 
> I think this naming inaccuracy resulted in Martin designing
> special_array_member in an inconsistent way, mixing size zero array members
> with array members of one or two or more elements and then using the
> size zero interchangeably with zero elements.
> 
> The following patch changes that (but doesn't do any
> documentation/diagnostics renaming, as this is really a corner case),
> such that int_0/trail_0 for consistency is just about [0] arrays
> plus [] for the latter, not one or more zero sized elements case.
> 
> The testcase has one xfailed case for where perhaps in later GCC versions
> we could add extra code to handle it, for some reason we don't diagnose
> out of bounds accesses for the zero sized elements cases.  It will be
> harder because e.g. FRE will canonicalize &var.fld[0] and &var.fld[10]
> to just one of them because they are provably the same address.
> But the important thing is to fix this regression (where we warn on
> completely valid code in the Linux kernel).  Anyway, for further work
> on this we don't really need any extra help from special_array_member,
> all code can just check integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (type))),
> it doesn't depend on the position of the members etc.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2023-03-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/109215
> 	* tree.h (enum special_array_member): Adjust comments for int_0
> 	and trail_0.
> 	* tree.cc (component_ref_sam_type): Clear zero_elts if memtype
> 	has zero sized element type and the array has variable number of
> 	elements or constant one or more elements.
> 	(component_ref_size): Adjust comments, formatting fix.
> 
> 	* gcc.dg/Wzero-length-array-bounds-3.c: New test.
> 
> --- gcc/tree.h.jj	2023-03-14 19:11:52.296936422 +0100
> +++ gcc/tree.h	2023-03-20 18:48:23.068788830 +0100
> @@ -5579,8 +5579,8 @@ extern tree component_ref_field_offset (
>  enum struct special_array_member
>    {
>      none,	/* Not a special array member.  */
> -    int_0,	/* Interior array member with size zero.  */
> -    trail_0,	/* Trailing array member with size zero.  */
> +    int_0,	/* Interior array member with zero elements.  */
> +    trail_0,	/* Trailing array member with zero elements.  */
>      trail_1,	/* Trailing array member with one element.  */
>      trail_n,	/* Trailing array member with two or more elements.  */
>      int_n	/* Interior array member with one or more elements.  */
> --- gcc/tree.cc.jj	2023-03-10 10:38:46.551473829 +0100
> +++ gcc/tree.cc	2023-03-20 19:41:35.605580732 +0100
> @@ -13032,14 +13032,27 @@ component_ref_sam_type (tree ref)
>  	return sam_type;
>  
>        bool trailing = false;
> -      (void)array_ref_flexible_size_p (ref, &trailing);
> -      bool zero_length = integer_zerop (memsize);
> -      if (!trailing && !zero_length)
> -	/* MEMBER is an interior array with
> -	  more than one element.  */
> +      (void) array_ref_flexible_size_p (ref, &trailing);
> +      bool zero_elts = integer_zerop (memsize);
> +      if (zero_elts && integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (memtype))))
> +	{
> +	  /* If array element has zero size, verify if it is a flexible
> +	     array member or zero length array.  Clear zero_elts if
> +	     it has one or more members or is a VLA member.  */
> +	  if (tree dom = TYPE_DOMAIN (memtype))
> +	    if (tree min = TYPE_MIN_VALUE (dom))
> +	      if (tree max = TYPE_MAX_VALUE (dom))
> +		if (TREE_CODE (min) != INTEGER_CST
> +		    || TREE_CODE (max) != INTEGER_CST
> +		    || !((integer_zerop (min) && integer_all_onesp (max))
> +			 || tree_int_cst_lt (max, min)))
> +		  zero_elts = false;
> +	}
> +      if (!trailing && !zero_elts)
> +	/* MEMBER is an interior array with more than one element.  */
>  	return special_array_member::int_n;
>  
> -      if (zero_length)
> +      if (zero_elts)
>  	{
>  	  if (trailing)
>  	    return special_array_member::trail_0;
> @@ -13047,7 +13060,7 @@ component_ref_sam_type (tree ref)
>  	    return special_array_member::int_0;
>  	}
>  
> -      if (!zero_length)
> +      if (!zero_elts)
>  	if (tree dom = TYPE_DOMAIN (memtype))
>  	  if (tree min = TYPE_MIN_VALUE (dom))
>  	    if (tree max = TYPE_MAX_VALUE (dom))
> @@ -13114,14 +13127,14 @@ component_ref_size (tree ref, special_ar
>  
>        tree afield_decl = TREE_OPERAND (ref, 1);
>        gcc_assert (TREE_CODE (afield_decl) == FIELD_DECL);
> -      /* if the trailing array is a not a flexible array member, treat it as
> +      /* If the trailing array is a not a flexible array member, treat it as
>  	 a normal array.  */
>        if (DECL_NOT_FLEXARRAY (afield_decl)
>  	  && *sam != special_array_member::int_0)
>  	return memsize;
>  
>        if (*sam == special_array_member::int_0)
> -	  memsize = NULL_TREE;
> +	memsize = NULL_TREE;
>  
>        /* For a reference to a flexible array member of a union
>  	 use the size of the union instead of the size of the member.  */
> @@ -13129,7 +13142,7 @@ component_ref_size (tree ref, special_ar
>  	memsize = TYPE_SIZE_UNIT (argtype);
>      }
>  
> -  /* MEMBER is either a bona fide flexible array member, or a zero-length
> +  /* MEMBER is either a bona fide flexible array member, or a zero-elements
>       array member, or an array of length one treated as such.  */
>  
>    /* If the reference is to a declared object and the member a true
> --- gcc/testsuite/gcc.dg/Wzero-length-array-bounds-3.c.jj	2023-03-20 19:17:21.290648885 +0100
> +++ gcc/testsuite/gcc.dg/Wzero-length-array-bounds-3.c	2023-03-20 19:16:48.494123583 +0100
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/109215 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wall" } */
> +
> +struct S {};
> +struct T { struct S s[3]; struct S t; };
> +void foo (struct S *);
> +
> +void
> +bar (struct T *t)
> +{
> +  foo (&t->s[2]);	/* { dg-bogus "array subscript 2 is outside the bounds of an interior zero-length array" } */
> +}
> +
> +void
> +baz (struct T *t)
> +{
> +  foo (&t->s[3]);	/* { dg-error "" "" { xfail *-*-* } } */
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)