Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Dominique Dhumieres]

> ...
> diff --git gcc/testsuite/c-c++-common/pr60226.c gcc/testsuite/c-c++-common/pr60226.c
> ...

The test fails on x86_64-apple-darwin13 with

FAIL: c-c++-common/pr60226.c  -std=gnu++98 (test for excess errors)
Excess errors:
/opt/gcc/work/gcc/testsuite/c-c++-common/pr60226.c:6:7: error: alignment of 'foo' is greater than maximum object file alignment 32768

Dominique

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Mike Stump]

On Jul 8, 2014, at 3:12 PM, Dominique Dhumieres <dominiq@lps.ens.fr> wrote:
>> diff --git gcc/testsuite/c-c++-common/pr60226.c gcc/testsuite/c-c++-common/pr60226.c
> 
> The test fails on x86_64-apple-darwin13 with
> 
> FAIL: c-c++-common/pr60226.c  -std=gnu++98 (test for excess errors)
> Excess errors:
> /opt/gcc/work/gcc/testsuite/c-c++-common/pr60226.c:6:7: error: alignment of 'foo' is greater than maximum object file alignment 32768

Fixed, thanks.

Index: pr60226.c
===================================================================
--- pr60226.c	(revision 212379)
+++ pr60226.c	(working copy)
@@ -4,7 +4,7 @@
 
 typedef int __attribute__ ((aligned (1 << 28))) int28;
 int28 foo[4] = {}; /* { dg-error "alignment of array elements is greater than element size" } */
-typedef int __attribute__ ((aligned (1 << 29))) int29; /* { dg-error "requested alignment is too large" } */
+typedef int __attribute__ ((aligned (1 << 29))) int29; /* { dg-error "requested alignment is too large|maximum object file alignment" } */
 
 void
 f (void)

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Jeff Law]

On 07/03/14 04:18, Marek Polacek wrote:
> On Mon, Jun 30, 2014 at 03:40:18PM -0700, Mike Stump wrote:
>> I glanced at it:
>>
>> (gdb) p/x TYPE_ALIGN (type)
>> $1 = 2147483648
>> (gdb) p/x TYPE_ALIGN (type)
>> $2 = 0x80000000
>>
>> The callee is int, the caller uses unsigned int.  The assert I see is because the routines are not type correct:
>>
>> =>    TYPE_SIZE (type) = round_up (TYPE_SIZE (type), TYPE_ALIGN (type));
>>
>> (gdb) ptype TYPE_ALIGN (type)
>> type = unsigned int
>>
>>
>> tree
>> round_up_loc (location_t loc, tree value, int divisor)
>> {
>>    tree div = NULL_TREE;
>>
>> =>gcc_assert (divisor > 0);
>>
>> Would be nice if the routine was type correct (wrt unsigned).
>
> Yeah, I did that.  One issue with that is that round_up now wraps
> the value, so I had to add a check for huge size before rounding up,
> otherwise we'd regress on e.g. PR42611.
>
> How about the following?
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2014-07-03  Marek Polacek  <polacek@redhat.com>
>
> 	PR c/60226
> 	* fold-const.c (round_up_loc): Change the parameter type.
> 	Remove assert.
> 	* fold-const.h (round_up_loc): Adjust declaration.
> 	* stor-layout.c (finalize_record_size): Check for too large types.
>
> 	* c-c++-common/pr60226.c: New test.
OK.
Jeff

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Marek Polacek]

On Mon, Jun 30, 2014 at 03:40:18PM -0700, Mike Stump wrote:
> I glanced at it:
> 
> (gdb) p/x TYPE_ALIGN (type)
> $1 = 2147483648
> (gdb) p/x TYPE_ALIGN (type)
> $2 = 0x80000000
> 
> The callee is int, the caller uses unsigned int.  The assert I see is because the routines are not type correct:
> 
> =>    TYPE_SIZE (type) = round_up (TYPE_SIZE (type), TYPE_ALIGN (type));
> 
> (gdb) ptype TYPE_ALIGN (type)
> type = unsigned int
> 
> 
> tree
> round_up_loc (location_t loc, tree value, int divisor)
> {
>   tree div = NULL_TREE;
> 
> =>gcc_assert (divisor > 0);
> 
> Would be nice if the routine was type correct (wrt unsigned).

Yeah, I did that.  One issue with that is that round_up now wraps
the value, so I had to add a check for huge size before rounding up,
otherwise we'd regress on e.g. PR42611.

How about the following?

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

2014-07-03  Marek Polacek  <polacek@redhat.com>

	PR c/60226
	* fold-const.c (round_up_loc): Change the parameter type.
	Remove assert.
	* fold-const.h (round_up_loc): Adjust declaration.
	* stor-layout.c (finalize_record_size): Check for too large types.

	* c-c++-common/pr60226.c: New test.

diff --git gcc/fold-const.c gcc/fold-const.c
index d22eac1..c57ac7b 100644
--- gcc/fold-const.c
+++ gcc/fold-const.c
@@ -16647,11 +16647,10 @@ fold_ignored_result (tree t)
 /* Return the value of VALUE, rounded up to a multiple of DIVISOR. */
 
 tree
-round_up_loc (location_t loc, tree value, int divisor)
+round_up_loc (location_t loc, tree value, unsigned int divisor)
 {
   tree div = NULL_TREE;
 
-  gcc_assert (divisor > 0);
   if (divisor == 1)
     return value;
 
diff --git gcc/fold-const.h gcc/fold-const.h
index dcb97a1..3b5fd84 100644
--- gcc/fold-const.h
+++ gcc/fold-const.h
@@ -144,7 +144,7 @@ extern tree combine_comparisons (location_t, enum tree_code, enum tree_code,
 extern void debug_fold_checksum (const_tree);
 extern bool may_negate_without_overflow_p (const_tree);
 #define round_up(T,N) round_up_loc (UNKNOWN_LOCATION, T, N)
-extern tree round_up_loc (location_t, tree, int);
+extern tree round_up_loc (location_t, tree, unsigned int);
 #define round_down(T,N) round_down_loc (UNKNOWN_LOCATION, T, N)
 extern tree round_down_loc (location_t, tree, int);
 extern tree size_int_kind (HOST_WIDE_INT, enum size_type_kind);
diff --git gcc/stor-layout.c gcc/stor-layout.c
index cfd436f..19e7adb 100644
--- gcc/stor-layout.c
+++ gcc/stor-layout.c
@@ -1587,6 +1587,11 @@ finalize_record_size (record_layout_info rli)
     unpadded_size_unit
       = size_binop (PLUS_EXPR, unpadded_size_unit, size_one_node);
 
+  if (TREE_CODE (unpadded_size_unit) == INTEGER_CST
+      && !TREE_OVERFLOW (unpadded_size_unit)
+      && !valid_constant_size_p (unpadded_size_unit))
+    error ("type %qT is too large", rli->t);
+
   /* Round the size up to be a multiple of the required alignment.  */
   TYPE_SIZE (rli->t) = round_up (unpadded_size, TYPE_ALIGN (rli->t));
   TYPE_SIZE_UNIT (rli->t)
diff --git gcc/testsuite/c-c++-common/pr60226.c gcc/testsuite/c-c++-common/pr60226.c
index e69de29..3a1c261 100644
--- gcc/testsuite/c-c++-common/pr60226.c
+++ gcc/testsuite/c-c++-common/pr60226.c
@@ -0,0 +1,14 @@
+/* PR c/60226 */
+/* { dg-do compile } */
+/* { dg-options "-Wno-c++-compat" { target c } } */
+
+typedef int __attribute__ ((aligned (1 << 28))) int28;
+int28 foo[4] = {}; /* { dg-error "alignment of array elements is greater than element size" } */
+typedef int __attribute__ ((aligned (1 << 29))) int29; /* { dg-error "requested alignment is too large" } */
+
+void
+f (void)
+{
+  struct { __attribute__((aligned (1 << 28))) double a; } x1;
+  struct { __attribute__((aligned (1 << 29))) double a; } x2; /* { dg-error "requested alignment is too large" } */
+}

	Marek

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Marek Polacek]

On Mon, Jun 30, 2014 at 01:50:12PM -0600, Jeff Law wrote:
> On 03/04/14 09:40, Marek Polacek wrote:
> >This should fix ICE on insane alignment.  Normally, check_user_alignment
> >detects e.g. alignment 1 << 32, but not 1 << 28.  However, record_align
> >is in bits, so it's actually 8 * (1 << 28) and that's greater than
> >INT_MAX.  This patch rejects such code.
> >
> >In the middle hunk, we should give up when an error occurs, we don't
> >want to call finalize_type_size in that case -- we'd ICE in there.
> >
> >Regtested/bootstrapped on x86_64-linux, ok for trunk?
> >
> >2014-03-04  Marek Polacek  <polacek@redhat.com>
> >
> >	PR middle-end/60226
> >	* stor-layout.c (layout_type): Return if alignment of array elements
> >	is greater than element size.  Error out if requested alignment is too
> >	large.
> >cp/
> >	* class.c (layout_class_type): Error out if requested alignment is too
> >	large.
> >testsuite/
> >	* c-c++-common/pr60226.c: New test.
> Is this still applicable after the wide-int changes?  I haven't looked
> closely.

Yeah, it applies cleanly.  But I tried the int -> unsigned change
which Mike suggested and that cures the ICE.  I'll send a patch
momentarily.

	Marek

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Mike Stump]

On Jun 30, 2014, at 3:40 PM, Mike Stump <mikestump@comcast.net> wrote:
>> Is this still applicable after the wide-int changes?  I haven't looked closely.

Oops, forgot to state what I wanted to state…  Yes, it still aborts post wide-int…

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Mike Stump]

On Jun 30, 2014, at 12:50 PM, Jeff Law <law@redhat.com> wrote:
> On 03/04/14 09:40, Marek Polacek wrote:
>> This should fix ICE on insane alignment.  Normally, check_user_alignment
>> detects e.g. alignment 1 << 32, but not 1 << 28.  However, record_align
>> is in bits, so it's actually 8 * (1 << 28) and that's greater than
>> INT_MAX.  This patch rejects such code.
>> 
>> In the middle hunk, we should give up when an error occurs, we don't
>> want to call finalize_type_size in that case -- we'd ICE in there.
>> 
>> Regtested/bootstrapped on x86_64-linux, ok for trunk?
>> 
>> 2014-03-04  Marek Polacek  <polacek@redhat.com>
>> 
>> 	PR middle-end/60226
>> 	* stor-layout.c (layout_type): Return if alignment of array elements
>> 	is greater than element size.  Error out if requested alignment is too
>> 	large.
>> cp/
>> 	* class.c (layout_class_type): Error out if requested alignment is too
>> 	large.
>> testsuite/
>> 	* c-c++-common/pr60226.c: New test.
> Is this still applicable after the wide-int changes?  I haven't looked closely.

I glanced at it:

(gdb) p/x TYPE_ALIGN (type)
$1 = 2147483648
(gdb) p/x TYPE_ALIGN (type)
$2 = 0x80000000

The callee is int, the caller uses unsigned int.  The assert I see is because the routines are not type correct:

=>    TYPE_SIZE (type) = round_up (TYPE_SIZE (type), TYPE_ALIGN (type));

(gdb) ptype TYPE_ALIGN (type)
type = unsigned int


tree
round_up_loc (location_t loc, tree value, int divisor)
{
  tree div = NULL_TREE;

=>gcc_assert (divisor > 0);

Would be nice if the routine was type correct (wrt unsigned).

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Jeff Law]

On 03/04/14 09:40, Marek Polacek wrote:
> This should fix ICE on insane alignment.  Normally, check_user_alignment
> detects e.g. alignment 1 << 32, but not 1 << 28.  However, record_align
> is in bits, so it's actually 8 * (1 << 28) and that's greater than
> INT_MAX.  This patch rejects such code.
>
> In the middle hunk, we should give up when an error occurs, we don't
> want to call finalize_type_size in that case -- we'd ICE in there.
>
> Regtested/bootstrapped on x86_64-linux, ok for trunk?
>
> 2014-03-04  Marek Polacek  <polacek@redhat.com>
>
> 	PR middle-end/60226
> 	* stor-layout.c (layout_type): Return if alignment of array elements
> 	is greater than element size.  Error out if requested alignment is too
> 	large.
> cp/
> 	* class.c (layout_class_type): Error out if requested alignment is too
> 	large.
> testsuite/
> 	* c-c++-common/pr60226.c: New test.
Is this still applicable after the wide-int changes?  I haven't looked 
closely.

jeff

Re: [PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Marek Polacek]

Ping.

On Tue, Mar 04, 2014 at 05:40:29PM +0100, Marek Polacek wrote:
> This should fix ICE on insane alignment.  Normally, check_user_alignment
> detects e.g. alignment 1 << 32, but not 1 << 28.  However, record_align
> is in bits, so it's actually 8 * (1 << 28) and that's greater than
> INT_MAX.  This patch rejects such code.
> 
> In the middle hunk, we should give up when an error occurs, we don't
> want to call finalize_type_size in that case -- we'd ICE in there.
> 
> Regtested/bootstrapped on x86_64-linux, ok for trunk?
> 
> 2014-03-04  Marek Polacek  <polacek@redhat.com>
> 
> 	PR middle-end/60226
> 	* stor-layout.c (layout_type): Return if alignment of array elements
> 	is greater than element size.  Error out if requested alignment is too
> 	large.
> cp/
> 	* class.c (layout_class_type): Error out if requested alignment is too
> 	large.
> testsuite/
> 	* c-c++-common/pr60226.c: New test.
> 
> diff --git gcc/cp/class.c gcc/cp/class.c
> index b46391b..e6325b3 100644
> --- gcc/cp/class.c
> +++ gcc/cp/class.c
> @@ -6378,6 +6378,14 @@ layout_class_type (tree t, tree *virtuals_p)
>    if (TYPE_PACKED (t) && !layout_pod_type_p (t))
>      rli->packed_maybe_necessary = true;
>  
> +  if (rli->record_align >= (1U << (HOST_BITS_PER_INT - 1)))
> +    {
> +      TYPE_SIZE (rli->t) = integer_zero_node;
> +      TYPE_SIZE_UNIT (rli->t) = integer_zero_node;
> +      error ("requested alignment is too large");
> +      return;
> +    }
> +
>    /* Let the back end lay out the type.  */
>    finish_record_layout (rli, /*free_p=*/true);
>  
> diff --git gcc/stor-layout.c gcc/stor-layout.c
> index 084d195..445f0d5 100644
> --- gcc/stor-layout.c
> +++ gcc/stor-layout.c
> @@ -2266,8 +2266,11 @@ layout_type (tree type)
>  	    && !TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
>  	    && !integer_zerop (TYPE_SIZE_UNIT (element))
>  	    && compare_tree_int (TYPE_SIZE_UNIT (element),
> -			  	 TYPE_ALIGN_UNIT (element)) < 0)
> -	  error ("alignment of array elements is greater than element size");
> +				 TYPE_ALIGN_UNIT (element)) < 0)
> +	  {
> +	    error ("alignment of array elements is greater than element size");
> +	    return;
> +	  }
>  	break;
>        }
>  
> @@ -2294,6 +2297,14 @@ layout_type (tree type)
>  	if (TREE_CODE (type) == QUAL_UNION_TYPE)
>  	  TYPE_FIELDS (type) = nreverse (TYPE_FIELDS (type));
>  
> +	if (rli->record_align >= (1U << (HOST_BITS_PER_INT - 1)))
> +	  {
> +	    TYPE_SIZE (rli->t) = integer_zero_node;
> +	    TYPE_SIZE_UNIT (rli->t) = integer_zero_node;
> +	    error ("requested alignment is too large");
> +	    return;
> +	  }
> +
>  	/* Finish laying out the record.  */
>  	finish_record_layout (rli, /*free_p=*/true);
>        }
> diff --git gcc/testsuite/c-c++-common/pr60226.c gcc/testsuite/c-c++-common/pr60226.c
> index e69de29..0d7d74d 100644
> --- gcc/testsuite/c-c++-common/pr60226.c
> +++ gcc/testsuite/c-c++-common/pr60226.c
> @@ -0,0 +1,12 @@
> +/* PR c/60226 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wno-c++-compat" { target c } } */
> +
> +typedef int __attribute__ ((aligned (1 << 28))) int28;
> +int28 foo[4] = {}; /* { dg-error "alignment of array elements is greater than element size" } */
> +
> +void
> +f (void)
> +{
> +  struct { __attribute__((aligned (1 << 28))) double a; } x; /* { dg-error "requested alignment is too large" } */
> +}
> 
> 	Marek

	Marek

[PATCH] Don't ICE with huge alignment (PR middle-end/60226)

[Marek Polacek]

This should fix ICE on insane alignment.  Normally, check_user_alignment
detects e.g. alignment 1 << 32, but not 1 << 28.  However, record_align
is in bits, so it's actually 8 * (1 << 28) and that's greater than
INT_MAX.  This patch rejects such code.

In the middle hunk, we should give up when an error occurs, we don't
want to call finalize_type_size in that case -- we'd ICE in there.

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2014-03-04  Marek Polacek  <polacek@redhat.com>

	PR middle-end/60226
	* stor-layout.c (layout_type): Return if alignment of array elements
	is greater than element size.  Error out if requested alignment is too
	large.
cp/
	* class.c (layout_class_type): Error out if requested alignment is too
	large.
testsuite/
	* c-c++-common/pr60226.c: New test.

diff --git gcc/cp/class.c gcc/cp/class.c
index b46391b..e6325b3 100644
--- gcc/cp/class.c
+++ gcc/cp/class.c
@@ -6378,6 +6378,14 @@ layout_class_type (tree t, tree *virtuals_p)
   if (TYPE_PACKED (t) && !layout_pod_type_p (t))
     rli->packed_maybe_necessary = true;
 
+  if (rli->record_align >= (1U << (HOST_BITS_PER_INT - 1)))
+    {
+      TYPE_SIZE (rli->t) = integer_zero_node;
+      TYPE_SIZE_UNIT (rli->t) = integer_zero_node;
+      error ("requested alignment is too large");
+      return;
+    }
+
   /* Let the back end lay out the type.  */
   finish_record_layout (rli, /*free_p=*/true);
 
diff --git gcc/stor-layout.c gcc/stor-layout.c
index 084d195..445f0d5 100644
--- gcc/stor-layout.c
+++ gcc/stor-layout.c
@@ -2266,8 +2266,11 @@ layout_type (tree type)
 	    && !TREE_OVERFLOW (TYPE_SIZE_UNIT (element))
 	    && !integer_zerop (TYPE_SIZE_UNIT (element))
 	    && compare_tree_int (TYPE_SIZE_UNIT (element),
-			  	 TYPE_ALIGN_UNIT (element)) < 0)
-	  error ("alignment of array elements is greater than element size");
+				 TYPE_ALIGN_UNIT (element)) < 0)
+	  {
+	    error ("alignment of array elements is greater than element size");
+	    return;
+	  }
 	break;
       }
 
@@ -2294,6 +2297,14 @@ layout_type (tree type)
 	if (TREE_CODE (type) == QUAL_UNION_TYPE)
 	  TYPE_FIELDS (type) = nreverse (TYPE_FIELDS (type));
 
+	if (rli->record_align >= (1U << (HOST_BITS_PER_INT - 1)))
+	  {
+	    TYPE_SIZE (rli->t) = integer_zero_node;
+	    TYPE_SIZE_UNIT (rli->t) = integer_zero_node;
+	    error ("requested alignment is too large");
+	    return;
+	  }
+
 	/* Finish laying out the record.  */
 	finish_record_layout (rli, /*free_p=*/true);
       }
diff --git gcc/testsuite/c-c++-common/pr60226.c gcc/testsuite/c-c++-common/pr60226.c
index e69de29..0d7d74d 100644
--- gcc/testsuite/c-c++-common/pr60226.c
+++ gcc/testsuite/c-c++-common/pr60226.c
@@ -0,0 +1,12 @@
+/* PR c/60226 */
+/* { dg-do compile } */
+/* { dg-options "-Wno-c++-compat" { target c } } */
+
+typedef int __attribute__ ((aligned (1 << 28))) int28;
+int28 foo[4] = {}; /* { dg-error "alignment of array elements is greater than element size" } */
+
+void
+f (void)
+{
+  struct { __attribute__((aligned (1 << 28))) double a; } x; /* { dg-error "requested alignment is too large" } */
+}

	Marek