[PATCH][C] Change array size overflow check

[PATCH][C] Change array size overflow check

[Richard Guenther]

This changes the code that deals with too large array sizes to
use int_fits_type_p instead of relying on the TREE_OVERFLOW setting
of the tree folder.  The latter will break once we don't treat
sizetypes specially (and they keep being unsigned).

Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?

Thanks,
Richard.

2011-05-02  Richard Guenther  <rguenther@suse.de>

	* c-decl.c (grokdeclarator): Instead of looking at
	TREE_OVERFLOW check if the constant fits in the index type.

Index: trunk/gcc/c-decl.c
===================================================================
*** trunk.orig/gcc/c-decl.c	2011-05-02 14:50:36.000000000 +0200
--- trunk/gcc/c-decl.c	2011-05-02 15:12:15.000000000 +0200
*************** grokdeclarator (const struct c_declarato
*** 5368,5382 ****
  					     convert (index_type,
  						      size_one_node));
  
! 		    /* If that overflowed, the array is too big.  ???
! 		       While a size of INT_MAX+1 technically shouldn't
! 		       cause an overflow (because we subtract 1), the
! 		       overflow is recorded during the conversion to
! 		       index_type, before the subtraction.  Handling
! 		       this case seems like an unnecessary
! 		       complication.  */
! 		    if (TREE_CODE (itype) == INTEGER_CST
! 			&& TREE_OVERFLOW (itype))
  		      {
  			if (name)
  			  error_at (loc, "size of array %qE is too large",
--- 5368,5380 ----
  					     convert (index_type,
  						      size_one_node));
  
! 		    /* The above overflows when size does not fit
! 		       in index_type.
! 		       ???  While a size of INT_MAX+1 technically shouldn't
! 		       cause an overflow (because we subtract 1), handling
! 		       this case seems like an unnecessary complication.  */
! 		    if (TREE_CODE (size) == INTEGER_CST
! 			&& !int_fits_type_p (size, index_type))
  		      {
  			if (name)
  			  error_at (loc, "size of array %qE is too large",

Re: [PATCH][C] Change array size overflow check

[Joseph S. Myers]

On Mon, 2 May 2011, Richard Guenther wrote:

> This changes the code that deals with too large array sizes to
> use int_fits_type_p instead of relying on the TREE_OVERFLOW setting
> of the tree folder.  The latter will break once we don't treat
> sizetypes specially (and they keep being unsigned).
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?

An array size in C or C++ ought to be considered to overflow (and so give 
an error if the size is compile-time constant) if the size of the array in 
bytes is greater than or equal to half the address space, because it is 
then no longer possible to compute differences between all array elements, 
and pointers to just past the end of the array, reliably as ptrdiff_t 
values (cf. PR 45779).  Thus, overflow in a signed rather than unsigned 
type is what's relevant.

I don't know if there's a relevant testcase in the testsuite, but the 
patch is OK with the addition of a testcase such as

/* { dg-do compile } */
/* { dg-options "" } */

typedef __SIZE_TYPE__ size_t;

extern char a[((size_t)-1 >> 1) + 1]; /* { dg-error "too large" } */
extern char b[((size_t)-1 >> 1)];
extern int c[(((size_t)-1 >> 1) + 1) / sizeof(int)]; /* { dg-error "too large" } */
extern int d[((size_t)-1 >> 1) / sizeof(int)];

supposing it passes.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][C] Change array size overflow check

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1882 bytes --]

On Mon, 2 May 2011, Joseph S. Myers wrote:

> On Mon, 2 May 2011, Richard Guenther wrote:
> 
> > This changes the code that deals with too large array sizes to
> > use int_fits_type_p instead of relying on the TREE_OVERFLOW setting
> > of the tree folder.  The latter will break once we don't treat
> > sizetypes specially (and they keep being unsigned).
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?
> 
> An array size in C or C++ ought to be considered to overflow (and so give 
> an error if the size is compile-time constant) if the size of the array in 
> bytes is greater than or equal to half the address space, because it is 
> then no longer possible to compute differences between all array elements, 
> and pointers to just past the end of the array, reliably as ptrdiff_t 
> values (cf. PR 45779).  Thus, overflow in a signed rather than unsigned 
> type is what's relevant.
> 
> I don't know if there's a relevant testcase in the testsuite, but the 
> patch is OK with the addition of a testcase such as
> 
> /* { dg-do compile } */
> /* { dg-options "" } */
> 
> typedef __SIZE_TYPE__ size_t;
> 
> extern char a[((size_t)-1 >> 1) + 1]; /* { dg-error "too large" } */
> extern char b[((size_t)-1 >> 1)];
> extern int c[(((size_t)-1 >> 1) + 1) / sizeof(int)]; /* { dg-error "too large" } */
> extern int d[((size_t)-1 >> 1) / sizeof(int)];
> 
> supposing it passes.

We do have similar testcases in gcc.dg/large-size-array*.c, but not
exactly testing the bound.  The above testcase also complains with
-pedantic about

t.c:3:13: error: size of array Β‘aΒ’ is too large
t.c:4:1: error: overflow in constant expression [-Woverflow]
t.c:5:12: error: size of array Β‘cΒ’ is too large
t.c:6:1: error: overflow in constant expression [-Woverflow]

with and without the patch.  I can add -Wno-overflow to the flags.
Ok with that?

Thanks,
Richard.

Re: [PATCH][C] Change array size overflow check

[Joseph S. Myers]

On Mon, 2 May 2011, Richard Guenther wrote:

> We do have similar testcases in gcc.dg/large-size-array*.c, but not
> exactly testing the bound.  The above testcase also complains with
> -pedantic about
> 
> t.c:3:13: error: size of array 'a' is too large
> t.c:4:1: error: overflow in constant expression [-Woverflow]
> t.c:5:12: error: size of array 'c' is too large
> t.c:6:1: error: overflow in constant expression [-Woverflow]
> 
> with and without the patch.  I can add -Wno-overflow to the flags.
> Ok with that?

Those overflow errors are bugs (the tests have no overflowing constant 
expression, it must be something the compiler is generating internally), 
and it seems they only appear for 64-bit compilation for me, not for 
32-bit, for some reason.  But as they are independent of the patch, OK 
with -Wno-overflow and with a PR filed for the bogus overflow errors.

(I get the overflow errors for a char array for a size of 
((size_t)-1 >> 4)+1 but not for ((size_t)-1 >> 4), i.e. 1ULL<<60 bytes is 
the point where the bogus errors start.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][C] Change array size overflow check

[Richard Guenther]

On Mon, 2 May 2011, Joseph S. Myers wrote:

> On Mon, 2 May 2011, Richard Guenther wrote:
> 
> > We do have similar testcases in gcc.dg/large-size-array*.c, but not
> > exactly testing the bound.  The above testcase also complains with
> > -pedantic about
> > 
> > t.c:3:13: error: size of array 'a' is too large
> > t.c:4:1: error: overflow in constant expression [-Woverflow]
> > t.c:5:12: error: size of array 'c' is too large
> > t.c:6:1: error: overflow in constant expression [-Woverflow]
> > 
> > with and without the patch.  I can add -Wno-overflow to the flags.
> > Ok with that?
> 
> Those overflow errors are bugs (the tests have no overflowing constant 
> expression, it must be something the compiler is generating internally), 
> and it seems they only appear for 64-bit compilation for me, not for 
> 32-bit, for some reason.  But as they are independent of the patch, OK 
> with -Wno-overflow and with a PR filed for the bogus overflow errors.
> 
> (I get the overflow errors for a char array for a size of 
> ((size_t)-1 >> 4)+1 but not for ((size_t)-1 >> 4), i.e. 1ULL<<60 bytes is 
> the point where the bogus errors start.)

Committed as rev. 173297.  I filed PR48850 for the rejects-valid.

Richard.