Fix up __builtin_alloca_with_align (0, ...) folding (PR sanitizer/91707)

Hi!

build_array_type_nelts is only meaningful for non-zero number of elements,
for 0 it creates weirdo arrays like char D.2358[0:18446744073709551615].
The following patch uses in that case types like the C FE emits for
zero-length array instead (i.e. char D.2358[0:] with forced 0 size).

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

2019-09-24  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/91707
	* tree-ssa-ccp.c (fold_builtin_alloca_with_align): For n_elem 0
	use a type like C zero length array instead of array from 0
	to SIZE_MAX.


	Jakub

On Tue, 24 Sep 2019, Jakub Jelinek wrote:

> Hi!
> 
> build_array_type_nelts is only meaningful for non-zero number of elements,
> for 0 it creates weirdo arrays like char D.2358[0:18446744073709551615].
> The following patch uses in that case types like the C FE emits for
> zero-length array instead (i.e. char D.2358[0:] with forced 0 size).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Not sure [0:-1] is actually the canonical zero-length array (and IIRC
what the C++ FE creates and what layout_type can lay out).  So why
not fix the sanitizers instead?

Richard.

> 2019-09-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/91707
> 	* tree-ssa-ccp.c (fold_builtin_alloca_with_align): For n_elem 0
> 	use a type like C zero length array instead of array from 0
> 	to SIZE_MAX.
> 
> --- gcc/tree-ssa-ccp.c.jj	2019-09-20 12:25:26.809718354 +0200
> +++ gcc/tree-ssa-ccp.c	2019-09-23 19:38:03.530722874 +0200
> @@ -2223,7 +2223,18 @@ fold_builtin_alloca_with_align (gimple *
>    /* Declare array.  */
>    elem_type = build_nonstandard_integer_type (BITS_PER_UNIT, 1);
>    n_elem = size * 8 / BITS_PER_UNIT;
> -  array_type = build_array_type_nelts (elem_type, n_elem);
> +  if (n_elem == 0)
> +    {
> +      /* For alloca (0), use array type similar to C zero-length arrays.  */
> +      tree range_type = build_range_type (sizetype, size_zero_node, NULL_TREE);
> +      array_type = build_array_type (elem_type, range_type);
> +      array_type = build_distinct_type_copy (TYPE_MAIN_VARIANT (array_type));
> +      TYPE_SIZE (array_type) = bitsize_zero_node;
> +      TYPE_SIZE_UNIT (array_type) = size_zero_node;
> +      SET_TYPE_STRUCTURAL_EQUALITY (array_type);
> +    }
> +  else
> +    array_type = build_array_type_nelts (elem_type, n_elem);
>    var = create_tmp_var (array_type);
>    SET_DECL_ALIGN (var, TREE_INT_CST_LOW (gimple_call_arg (stmt, 1)));
>    if (uid != 0)
> 
> 	Jakub
>

On Tue, Sep 24, 2019 at 01:15:46PM +0200, Richard Biener wrote:
> > build_array_type_nelts is only meaningful for non-zero number of elements,
> > for 0 it creates weirdo arrays like char D.2358[0:18446744073709551615].
> > The following patch uses in that case types like the C FE emits for
> > zero-length array instead (i.e. char D.2358[0:] with forced 0 size).
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Not sure [0:-1] is actually the canonical zero-length array (and IIRC
> what the C++ FE creates and what layout_type can lay out).  So why

You're right, patch withdrawn.

> not fix the sanitizers instead?

Well, the problem isn't in sanitizers, but jump threading and late warnings
that are warning even about code specialized by jump threading.
It could be indeed solved with __builtin_warning if we defer the late
warnings and ignore them inside of sanitization report only paths (if we can
detect them reliably, perhaps pass dominated by a failed ubsan or asan
sanitization check), or by making jump threading not try to optimize the
cold sanitization diagnostics parts.

	Jakub

On Tue, 24 Sep 2019, Jakub Jelinek wrote:

> On Tue, Sep 24, 2019 at 01:15:46PM +0200, Richard Biener wrote:
> > > build_array_type_nelts is only meaningful for non-zero number of elements,
> > > for 0 it creates weirdo arrays like char D.2358[0:18446744073709551615].
> > > The following patch uses in that case types like the C FE emits for
> > > zero-length array instead (i.e. char D.2358[0:] with forced 0 size).
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Not sure [0:-1] is actually the canonical zero-length array (and IIRC
> > what the C++ FE creates and what layout_type can lay out).  So why
> 
> You're right, patch withdrawn.
> 
> > not fix the sanitizers instead?
> 
> Well, the problem isn't in sanitizers, but jump threading and late warnings
> that are warning even about code specialized by jump threading.
> It could be indeed solved with __builtin_warning if we defer the late
> warnings and ignore them inside of sanitization report only paths (if we can
> detect them reliably, perhaps pass dominated by a failed ubsan or asan
> sanitization check), or by making jump threading not try to optimize the
> cold sanitization diagnostics parts.

Hmm yeah.

Note that in principle the domain could be signed so that the
-1 is more obvious.  Also [1:0] would be an equally valid empty
domain.  Not sure if that helps the specific jump-threading case, of 
course...

Richard.

On Tue, Sep 24, 2019 at 03:10:49PM +0200, Richard Biener wrote:
> Hmm yeah.
> 
> Note that in principle the domain could be signed so that the
> -1 is more obvious.  Also [1:0] would be an equally valid empty
> domain.  Not sure if that helps the specific jump-threading case, of 
> course...

No, that doesn't help.
The code is essentially
void
foo (int x)
{
  if (x == 0)
    bar ();
  int v[x];
  v[0] = 1;
  if (x == 0)
    bar ();
}
where if jump threading creates
if (x == 0) { bar (); int v[0]; v[0] = 1; bar (); }
else { int v[x]; v[0] = 1; }
out of it, we do warn.  Whether we should warn in that case is something for
ongoing debate (I don't like such warnings, because the if (x == 0) doesn't
necessarily mean the code will be called with such arguments, it might be
just that something written generically got inlined in, but others like them
(Martin, Jeff)), in this specific case it is even that the if (x == 0) bar ();
doesn't actually come from the user code at all, but from the sanitization
and so even less desirable, because, well, user code didn't have any tests
like that at all.

	Jakub