array-bounds: Fix up ICE on overaligned variables [PR99109]

Hi!

check_mem_ref builds artificial arrays for variables that don't have
array type.
The C standard says:
"For the purposes of these operators, a pointer to an object that is not an element of an
array behaves the same as a pointer to the first element of an array of length one with the
type of the object as its element type."
so it isn't completely wrong and does simplify the function.
But, layout_type can fail if the size of the element type is not a multiple
of its alignment (i.e. overaligned types) and we then ICE because of that.

The following patch uses TYPE_MAIN_VARIANT in those cases instead, but only
for the types that need it, as for the diagnostics it is better to use the
typedef names etc. that were really used in the source if possible.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2021-02-17  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/99109
	* gimple-array-bounds.cc (overaligned_type_p): New function.
	(array_bounds_checker::check_mem_ref): For overaligned types use
	TYPE_MAIN_VARIANT of reftype as element type of a new array.

	* g++.dg/warn/Warray-bounds-17.C: New test.


	Jakub

On 2/17/21 3:12 AM, Jakub Jelinek via Gcc-patches wrote:
> Hi!
> 
> check_mem_ref builds artificial arrays for variables that don't have
> array type.
> The C standard says:
> "For the purposes of these operators, a pointer to an object that is not an element of an
> array behaves the same as a pointer to the first element of an array of length one with the
> type of the object as its element type."
> so it isn't completely wrong and does simplify the function.
> But, layout_type can fail if the size of the element type is not a multiple
> of its alignment (i.e. overaligned types) and we then ICE because of that.
> 
> The following patch uses TYPE_MAIN_VARIANT in those cases instead, but only
> for the types that need it, as for the diagnostics it is better to use the
> typedef names etc. that were really used in the source if possible.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2021-02-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/99109
> 	* gimple-array-bounds.cc (overaligned_type_p): New function.
> 	(array_bounds_checker::check_mem_ref): For overaligned types use
> 	TYPE_MAIN_VARIANT of reftype as element type of a new array.
> 
> 	* g++.dg/warn/Warray-bounds-17.C: New test.
> 
> --- gcc/gimple-array-bounds.cc.jj	2021-01-04 10:25:37.471249246 +0100
> +++ gcc/gimple-array-bounds.cc	2021-02-16 17:00:41.961750114 +0100
> @@ -386,6 +386,21 @@ build_zero_elt_array_type (tree eltype)
>     return arrtype;
>   }
>   
> +/* Return true if it is not possible to build an array with element type
> +   ELTYPE, because either ELTYPE has alignment larger than its size
> +   or its size is not a multiple of its alignment.  */
> +
> +static bool
> +overaligned_type_p (tree eltype)
> +{
> +  return (TYPE_SIZE_UNIT (eltype)
> +	  && TREE_CODE (TYPE_SIZE_UNIT (eltype)) == INTEGER_CST
> +	  && !integer_zerop (TYPE_SIZE_UNIT (eltype))
> +	  && TYPE_ALIGN_UNIT (eltype) > 1
> +	  && wi::zext (wi::to_wide (TYPE_SIZE_UNIT (eltype)),
> +		       ffs_hwi (TYPE_ALIGN_UNIT (eltype)) - 1) != 0);
> +}
> +
>   /* Checks one MEM_REF in REF, located at LOCATION, for out-of-bounds
>      references to string constants.  If VRP can determine that the array
>      subscript is a constant, check if it is outside valid range.
> @@ -553,6 +568,8 @@ array_bounds_checker::check_mem_ref (loc
>         reftype = TREE_TYPE (TREE_TYPE (arg));
>         if (TREE_CODE (reftype) == ARRAY_TYPE)
>   	reftype = TREE_TYPE (reftype);
> +      else if (overaligned_type_p (reftype))
> +	reftype = TYPE_MAIN_VARIANT (reftype);
>         if (tree refsize = TYPE_SIZE_UNIT (reftype))
>   	if (TREE_CODE (refsize) == INTEGER_CST)
>   	  eltsize = wi::to_offset (refsize);
> @@ -675,7 +692,11 @@ array_bounds_checker::check_mem_ref (loc
>         /* Treat a reference to a non-array object as one to an array
>   	 of a single element.  */
>         if (TREE_CODE (reftype) != ARRAY_TYPE)
> -	reftype = build_array_type_nelts (reftype, 1);
> +	{
> +	  if (overaligned_type_p (reftype))
> +	    reftype = TYPE_MAIN_VARIANT (reftype);
> +	  reftype = build_array_type_nelts (reftype, 1);
> +	}

Rather than complicating the logic in the caller (which is already
long and hard to follow) I'd suggest to consider changing
the build_zero_elt_array_type() helper to handle both kinds of arrays
(i.e., zero-length and otherwise), and strip the excess alignment
from reftype in it.  That will simplify the function.

I recall considering this (using a single helper) but not why I didn't
do it to begin with.

Martin

>   
>         /* Extract the element type out of MEM_REF and use its size
>   	 to compute the index to print in the diagnostic; arrays
> --- gcc/testsuite/g++.dg/warn/Warray-bounds-17.C.jj	2021-02-16 17:24:14.178813304 +0100
> +++ gcc/testsuite/g++.dg/warn/Warray-bounds-17.C	2021-02-16 17:23:35.305251062 +0100
> @@ -0,0 +1,15 @@
> +// PR middle-end/99109
> +// { dg-do compile }
> +// { dg-options "-O2 -Warray-bounds" }
> +
> +typedef int A __attribute__((aligned (64)));
> +void foo (int *);
> +
> +void
> +bar (void)
> +{
> +  A b;			// { dg-message "while referencing" }
> +  int *p = &b;
> +  int *x = (p - 1);	// { dg-warning "outside array bounds" }
> +  foo (x);
> +}
> 
> 	Jakub
>

On Wed, Feb 17, 2021 at 01:38:56PM -0700, Martin Sebor wrote:
> > -	reftype = build_array_type_nelts (reftype, 1);
> > +	{
> > +	  if (overaligned_type_p (reftype))
> > +	    reftype = TYPE_MAIN_VARIANT (reftype);
> > +	  reftype = build_array_type_nelts (reftype, 1);
> > +	}
> 
> Rather than complicating the logic in the caller (which is already
> long and hard to follow) I'd suggest to consider changing
> the build_zero_elt_array_type() helper to handle both kinds of arrays
> (i.e., zero-length and otherwise), and strip the excess alignment
> from reftype in it.  That will simplify the function.

That will mean the overaligned type checking will need to be done
even for the case where reftype was originally an ARRAY_TYPE and the code
just picks up an element type out of it (in that case it is known that it
will succeed).
And there is a question how to name it, build_array_type_nelts is already
taken and it might be confusing what this almost like build_array_type_nelts
but with extras differs from the tree.c function.

Anyway, can change it if you can suggest a good name...

	Jakub

On 2/17/21 1:56 PM, Jakub Jelinek wrote:
> On Wed, Feb 17, 2021 at 01:38:56PM -0700, Martin Sebor wrote:
>>> -	reftype = build_array_type_nelts (reftype, 1);
>>> +	{
>>> +	  if (overaligned_type_p (reftype))
>>> +	    reftype = TYPE_MAIN_VARIANT (reftype);
>>> +	  reftype = build_array_type_nelts (reftype, 1);
>>> +	}
>>
>> Rather than complicating the logic in the caller (which is already
>> long and hard to follow) I'd suggest to consider changing
>> the build_zero_elt_array_type() helper to handle both kinds of arrays
>> (i.e., zero-length and otherwise), and strip the excess alignment
>> from reftype in it.  That will simplify the function.
> 
> That will mean the overaligned type checking will need to be done
> even for the case where reftype was originally an ARRAY_TYPE and the code
> just picks up an element type out of it (in that case it is known that it
> will succeed).
> And there is a question how to name it, build_array_type_nelts is already
> taken and it might be confusing what this almost like build_array_type_nelts
> but with extras differs from the tree.c function.
> 
> Anyway, can change it if you can suggest a good name...

How does build_printable_array_type sound?

Also, would using TYPE_MAIN_VARIANT whenever TYPE_USER_ALIGN is set
be a simpler solution?  (It might not be as refined as the test in
your patch but I don't think we'd be giving up too much by
the simplification.)

Martin

On Wed, Feb 17, 2021 at 02:38:04PM -0700, Martin Sebor wrote:
> How does build_printable_array_type sound?

I'll go with that.

> Also, would using TYPE_MAIN_VARIANT whenever TYPE_USER_ALIGN is set
> be a simpler solution?  (It might not be as refined as the test in
> your patch but I don't think we'd be giving up too much by
> the simplification.)

That seems like a bad idea.  TYPE_USER_ALIGN is set quite often,
but in most cases the type size is a multiple of the alignment.

	Jakub