[C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 4563 bytes --]

Hi!

The following patches (included or attached) fix a regression on WebKit
compilation.
The first hunk basically reverts Honza's patch from December/January,
because layout_type when the variants are already linked in doesn't layout
just the current type, but also forcefully overwrites all the other
variants, which is clearly highly undesirable.  The attached
patch has an alternate hunk for that, where it doesn't call layout_type at
all and just copies over the needed fields from the main variant.

The second hunk (the same in between both patches) fixes a problem that
alignof (const T) is incorrect, but it has been that way already in 4.8 at
least.

I've been wondering if it would be possible that build_cplus_array_type
would see incomplete element type on the main variant and complete qualified
element type, but have not succeeded with e.g.
struct S
{
  typedef S T[4][4] __attribute__((aligned (16)));
  static T t;
  static volatile T v;
};
void foo (const S::T);
volatile const S::T w;
S::T S::t;
volatile S::T S::v;

The included (first) patch has been successfully bootstrapped/regtested on
x86_64-linux and i686-linux, the attached patch not, but I can
bootstrap/regtest it if you prefer it.

2015-04-08  Jakub Jelinek  <jakub@redhat.com>
	    Jan Hubicka  <hubicka@ucw.cz>

	PR c++/65690
	* tree.c (build_cplus_array_type): Layout type before variants are
	set, but copy over TYPE_SIZE and TYPE_SIZE_UNIT from the main
	variant.
	(cp_build_qualified_type_real): Use check_base_type.  Build a
	variant and copy over even TYPE_CONTEXT and
	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.

	* c-c++-common/attr-aligned-1.c: New test.

--- gcc/cp/tree.c.jj	2015-04-01 15:29:33.000000000 +0200
+++ gcc/cp/tree.c	2015-04-08 09:09:45.326939354 +0200
@@ -880,12 +880,19 @@ build_cplus_array_type (tree elt_type, t
 	{
 	  t = build_min_array_type (elt_type, index_type);
 	  set_array_type_canon (t, elt_type, index_type);
+	  if (!dependent)
+	    {
+	      layout_type (t);
+	      /* Make sure sizes are shared with the main variant.
+		 layout_type can't be called after setting TYPE_NEXT_VARIANT,
+		 as it will overwrite alignment etc. of all variants.  */
+	      TYPE_SIZE (t) = TYPE_SIZE (m);
+	      TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+	    }
 
 	  TYPE_MAIN_VARIANT (t) = m;
 	  TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (m);
 	  TYPE_NEXT_VARIANT (m) = t;
-	  if (!dependent)
-	    layout_type (t);
 	}
     }
 
@@ -1057,21 +1064,23 @@ cp_build_qualified_type_real (tree type,
 	 should be equivalent to those in check_qualified_type.  */
       for (t = TYPE_MAIN_VARIANT (type); t; t = TYPE_NEXT_VARIANT (t))
 	if (TREE_TYPE (t) == element_type
-	    && TYPE_NAME (t) == TYPE_NAME (type)
-	    && TYPE_CONTEXT (t) == TYPE_CONTEXT (type)
-	    && attribute_list_equal (TYPE_ATTRIBUTES (t),
-				     TYPE_ATTRIBUTES (type)))
+	    && check_base_type (t, type))
 	  break;
 
       if (!t)
 	{
 	  t = build_cplus_array_type (element_type, TYPE_DOMAIN (type));
 
-	  /* Keep the typedef name.  */
-	  if (TYPE_NAME (t) != TYPE_NAME (type))
+	  /* Keep the typedef name, context and alignment.  */
+	  if (TYPE_NAME (t) != TYPE_NAME (type)
+	      || TYPE_CONTEXT (t) != TYPE_CONTEXT (type)
+	      || TYPE_ALIGN (t) != TYPE_ALIGN (type))
 	    {
 	      t = build_variant_type_copy (t);
 	      TYPE_NAME (t) = TYPE_NAME (type);
+	      TYPE_CONTEXT (t) = TYPE_CONTEXT (type);
+	      TYPE_ALIGN (t) = TYPE_ALIGN (type);
+	      TYPE_USER_ALIGN (t) = TYPE_USER_ALIGN (type);
 	    }
 	}
 
--- gcc/testsuite/c-c++-common/attr-aligned-1.c.jj	2015-04-08 09:22:46.181427189 +0200
+++ gcc/testsuite/c-c++-common/attr-aligned-1.c	2015-04-08 09:26:41.315627195 +0200
@@ -0,0 +1,24 @@
+/* PR c++/65690 */
+/* { dg-do run } */
+
+typedef double T[4][4] __attribute__((aligned (2 * __alignof__ (double))));
+void foo (const T);
+struct S { T s; };
+
+int
+main ()
+{
+  if (__alignof__ (struct S) != 2 * __alignof__ (double)
+      || __alignof__ (T) != 2 * __alignof__ (double)
+      || __alignof__ (const struct S) != 2 * __alignof__ (double)
+      || __alignof__ (const T) != 2 * __alignof__ (double))
+    __builtin_abort ();
+  return 0;
+}
+
+#if defined(__cplusplus) && __cplusplus >= 201103L
+static_assert (alignof (S) == 2 * alignof (double), "alignment of S");
+static_assert (alignof (T) == 2 * alignof (double), "alignment of T");
+static_assert (alignof (const S) == 2 * alignof (double), "alignment of const S");
+static_assert (alignof (const T) == 2 * alignof (double), "alignment of const T");
+#endif

	Jakub

[-- Attachment #2: V554a --]
[-- Type: text/plain, Size: 3282 bytes --]

2015-04-08  Jakub Jelinek  <jakub@redhat.com>
	    Jan Hubicka  <hubicka@ucw.cz>

	PR c++/65690
	* tree.c (build_cplus_array_type): Don't layout the type,
	instead copy everything over from the main variant.
	(cp_build_qualified_type_real): Use check_base_type.  Build a
	variant and copy over even TYPE_CONTEXT and
	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.

	* c-c++-common/attr-aligned-1.c: New test.

--- gcc/cp/tree.c.jj	2015-04-01 15:29:33.000000000 +0200
+++ gcc/cp/tree.c	2015-04-08 09:09:45.326939354 +0200
@@ -880,12 +880,19 @@ build_cplus_array_type (tree elt_type, t
 	{
 	  t = build_min_array_type (elt_type, index_type);
 	  set_array_type_canon (t, elt_type, index_type);
+	  if (!dependent)
+	    {
+	      TYPE_SIZE (t) = TYPE_SIZE (m);
+	      TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+	      TYPE_ALIGN (t) = TYPE_ALIGN (m);
+	      TYPE_PRECISION (t) = TYPE_PRECISION (m);
+	      TYPE_USER_ALIGN (t) = TYPE_USER_ALIGN (m);
+	      SET_TYPE_MODE (t, TYPE_MODE (m));
+	    }
 
 	  TYPE_MAIN_VARIANT (t) = m;
 	  TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (m);
 	  TYPE_NEXT_VARIANT (m) = t;
-	  if (!dependent)
-	    layout_type (t);
 	}
     }
 
@@ -1057,21 +1064,23 @@ cp_build_qualified_type_real (tree type,
 	 should be equivalent to those in check_qualified_type.  */
       for (t = TYPE_MAIN_VARIANT (type); t; t = TYPE_NEXT_VARIANT (t))
 	if (TREE_TYPE (t) == element_type
-	    && TYPE_NAME (t) == TYPE_NAME (type)
-	    && TYPE_CONTEXT (t) == TYPE_CONTEXT (type)
-	    && attribute_list_equal (TYPE_ATTRIBUTES (t),
-				     TYPE_ATTRIBUTES (type)))
+	    && check_base_type (t, type))
 	  break;
 
       if (!t)
 	{
 	  t = build_cplus_array_type (element_type, TYPE_DOMAIN (type));
 
-	  /* Keep the typedef name.  */
-	  if (TYPE_NAME (t) != TYPE_NAME (type))
+	  /* Keep the typedef name, context and alignment.  */
+	  if (TYPE_NAME (t) != TYPE_NAME (type)
+	      || TYPE_CONTEXT (t) != TYPE_CONTEXT (type)
+	      || TYPE_ALIGN (t) != TYPE_ALIGN (type))
 	    {
 	      t = build_variant_type_copy (t);
 	      TYPE_NAME (t) = TYPE_NAME (type);
+	      TYPE_CONTEXT (t) = TYPE_CONTEXT (type);
+	      TYPE_ALIGN (t) = TYPE_ALIGN (type);
+	      TYPE_USER_ALIGN (t) = TYPE_USER_ALIGN (type);
 	    }
 	}
 
--- gcc/testsuite/c-c++-common/attr-aligned-1.c.jj	2015-04-08 09:22:46.181427189 +0200
+++ gcc/testsuite/c-c++-common/attr-aligned-1.c	2015-04-08 09:26:41.315627195 +0200
@@ -0,0 +1,24 @@
+/* PR c++/65690 */
+/* { dg-do run } */
+
+typedef double T[4][4] __attribute__((aligned (2 * __alignof__ (double))));
+void foo (const T);
+struct S { T s; };
+
+int
+main ()
+{
+  if (__alignof__ (struct S) != 2 * __alignof__ (double)
+      || __alignof__ (T) != 2 * __alignof__ (double)
+      || __alignof__ (const struct S) != 2 * __alignof__ (double)
+      || __alignof__ (const T) != 2 * __alignof__ (double))
+    __builtin_abort ();
+  return 0;
+}
+
+#if defined(__cplusplus) && __cplusplus >= 201103L
+static_assert (alignof (S) == 2 * alignof (double), "alignment of S");
+static_assert (alignof (T) == 2 * alignof (double), "alignment of T");
+static_assert (alignof (const S) == 2 * alignof (double), "alignment of const S");
+static_assert (alignof (const T) == 2 * alignof (double), "alignment of const T");
+#endif

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jason Merrill]

On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> 	(cp_build_qualified_type_real): Use check_base_type.  Build a
> 	variant and copy over even TYPE_CONTEXT and
> 	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.

This seems wrong.  If there is an array with the same name, attributes 
and element type, it should have the same alignment; if it doesn't, that 
probably means that one of the types hasn't been laid out yet.  We don't 
want to have two variants of the same array that are distinguished only 
by whether they've been laid out, especially since later probably both 
will be laid out and the two types will be the same.

Jason

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 10:47:15AM -0400, Jason Merrill wrote:
> On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> >	(cp_build_qualified_type_real): Use check_base_type.  Build a
> >	variant and copy over even TYPE_CONTEXT and
> >	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.
> 
> This seems wrong.  If there is an array with the same name, attributes and
> element type, it should have the same alignment; if it doesn't, that
> probably means that one of the types hasn't been laid out yet.  We don't
> want to have two variants of the same array that are distinguished only by
> whether they've been laid out, especially since later probably both will be
> laid out and the two types will be the same.

But that is how handle_aligned_attribute works, since forever (checked it
back to 3.2).  In <= 3.4.x, it used to create it using build_type_copy,
since 4.0.0 using build_variant_type_copy, but both those routines behave
the same - build a type variant which is linked in the TYPE_NEXT_VARIANT
chain, and differs from the other type in there possibly just by
TYPE_ALIGN/TYPE_USER_ALIGN.  Perhaps it should check TYPE_ALIGN only if
at least one of the two types has TYPE_USER_ALIGN set?
As for why TYPE_ATTRIBUTES are NULL, the reason for that is that
these are attributes on a typedef, so the attributes go into DECL_ATTRIBUTES
of the TYPE_DECL instead.

Anyway, the P1 regression is just about the first hunk, so if you have
issues just with the second hunk and not the first hunk (from either of the
patches), I can just comment out the tests for alignof (const T), and open
a separate PR for that for later.

	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jason Merrill]

On 04/08/2015 11:07 AM, Jakub Jelinek wrote:
> But that is how handle_aligned_attribute works, since forever (checked it
> back to 3.2).  In <= 3.4.x, it used to create it using build_type_copy,
> since 4.0.0 using build_variant_type_copy, but both those routines behave
> the same - build a type variant which is linked in the TYPE_NEXT_VARIANT
> chain, and differs from the other type in there possibly just by
> TYPE_ALIGN/TYPE_USER_ALIGN.  Perhaps it should check TYPE_ALIGN only if
> at least one of the two types has TYPE_USER_ALIGN set?
> As for why TYPE_ATTRIBUTES are NULL, the reason for that is that
> these are attributes on a typedef, so the attributes go into DECL_ATTRIBUTES
> of the TYPE_DECL instead.

I see.  So yes, I think we want to look at TYPE_USER_ALIGN as well.

> Anyway, the P1 regression is just about the first hunk, so if you have
> issues just with the second hunk and not the first hunk (from either of the
> patches), I can just comment out the tests for alignof (const T), and open
> a separate PR for that for later.

The second patch seems to cause a regression on c-c++-common/pr60226.c 
because we avoid calling layout_type, so we lose the over-alignment 
error.  So the first hunk of the first patch is OK.

Incidentally, for template testcases it seems we also need to fix 
layout_type and fixup_type_sizes to respect already-set TYPE_USER_ALIGN:

template <class T> struct A
{
   T t;
};

typedef A<int> T[4] alignas (2 * alignof (int));
A<int> a[4];

typedef A<char> T2[4] alignas (2 * alignof (int));

#define SA(X) static_assert((X),#X)
SA(alignof (T) == 2 * alignof(int));
SA(alignof (T2) == 2 * alignof(int));

Jason

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

On Thu, Apr 09, 2015 at 10:48:29AM -0400, Jason Merrill wrote:
> So the first hunk of the first patch is OK.

Ok, I've committed this to fix the regression.

2015-04-09  Jakub Jelinek  <jakub@redhat.com>

	PR c++/65690
	* tree.c (build_cplus_array_type): Layout type before variants are
	set, but copy over TYPE_SIZE and TYPE_SIZE_UNIT from the main
	variant.

	* c-c++-common/attr-aligned-1.c: New test.

--- gcc/cp/tree.c.jj	2015-04-01 15:29:33.000000000 +0200
+++ gcc/cp/tree.c	2015-04-08 09:09:45.326939354 +0200
@@ -880,12 +880,19 @@ build_cplus_array_type (tree elt_type, t
 	{
 	  t = build_min_array_type (elt_type, index_type);
 	  set_array_type_canon (t, elt_type, index_type);
+	  if (!dependent)
+	    {
+	      layout_type (t);
+	      /* Make sure sizes are shared with the main variant.
+		 layout_type can't be called after setting TYPE_NEXT_VARIANT,
+		 as it will overwrite alignment etc. of all variants.  */
+	      TYPE_SIZE (t) = TYPE_SIZE (m);
+	      TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+	    }
 
 	  TYPE_MAIN_VARIANT (t) = m;
 	  TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (m);
 	  TYPE_NEXT_VARIANT (m) = t;
-	  if (!dependent)
-	    layout_type (t);
 	}
     }
 
--- gcc/testsuite/c-c++-common/attr-aligned-1.c.jj	2015-04-08 09:22:46.181427189 +0200
+++ gcc/testsuite/c-c++-common/attr-aligned-1.c	2015-04-08 09:26:41.315627195 +0200
@@ -0,0 +1,22 @@
+/* PR c++/65690 */
+/* { dg-do run } */
+
+typedef double T[4][4] __attribute__((aligned (2 * __alignof__ (double))));
+void foo (const T);
+struct S { T s; };
+
+int
+main ()
+{
+  if (__alignof__ (struct S) != 2 * __alignof__ (double)
+      || __alignof__ (T) != 2 * __alignof__ (double)
+      || __alignof__ (const struct S) != 2 * __alignof__ (double))
+    __builtin_abort ();
+  return 0;
+}
+
+#if defined(__cplusplus) && __cplusplus >= 201103L
+static_assert (alignof (S) == 2 * alignof (double), "alignment of S");
+static_assert (alignof (T) == 2 * alignof (double), "alignment of T");
+static_assert (alignof (const S) == 2 * alignof (double), "alignment of const S");
+#endif

	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jan Hubicka]

> On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> >	(cp_build_qualified_type_real): Use check_base_type.  Build a
> >	variant and copy over even TYPE_CONTEXT and
> >	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.
> 
> This seems wrong.  If there is an array with the same name,
> attributes and element type, it should have the same alignment; if

One of problems is that cp_build_qualified_type rebuilds the array from
scratch and never copies the attribute list around (as oposed to
build_qualified_type that just memcpy the type node)

Honza

> it doesn't, that probably means that one of the types hasn't been
> laid out yet.  We don't want to have two variants of the same array
> that are distinguished only by whether they've been laid out,
> especially since later probably both will be laid out and the two
> types will be the same.
> 
> Jason

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 06:22:10PM +0200, Jan Hubicka wrote:
> > On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> > >	(cp_build_qualified_type_real): Use check_base_type.  Build a
> > >	variant and copy over even TYPE_CONTEXT and
> > >	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.
> > 
> > This seems wrong.  If there is an array with the same name,
> > attributes and element type, it should have the same alignment; if
> 
> One of problems is that cp_build_qualified_type rebuilds the array from
> scratch and never copies the attribute list around (as oposed to
> build_qualified_type that just memcpy the type node)

As I said earlier, TYPE_ATTRIBUTES is NULL here anyway, because the
attributes hang in DECL_ATTRIBUTES of TYPE_DECL.  And, except for
config/sol2.c (which looks wrong), nothing ever calls lookup_attribute for
"aligned" anyway, the user aligned stuff is encoded in TYPE_USER_ALIGN
and/or DECL_USER_ALIGN and TYPE_ALIGN/DECL_ALIGN.

	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jan Hubicka]

> On Wed, Apr 08, 2015 at 06:22:10PM +0200, Jan Hubicka wrote:
> > > On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> > > >	(cp_build_qualified_type_real): Use check_base_type.  Build a
> > > >	variant and copy over even TYPE_CONTEXT and
> > > >	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.
> > > 
> > > This seems wrong.  If there is an array with the same name,
> > > attributes and element type, it should have the same alignment; if
> > 
> > One of problems is that cp_build_qualified_type rebuilds the array from
> > scratch and never copies the attribute list around (as oposed to
> > build_qualified_type that just memcpy the type node)
> 
> As I said earlier, TYPE_ATTRIBUTES is NULL here anyway, because the
> attributes hang in DECL_ATTRIBUTES of TYPE_DECL.  And, except for
> config/sol2.c (which looks wrong), nothing ever calls lookup_attribute for
> "aligned" anyway, the user aligned stuff is encoded in TYPE_USER_ALIGN
> and/or DECL_USER_ALIGN and TYPE_ALIGN/DECL_ALIGN.

This is interesting too.  I did know that alignment is "lowered" into
TYPE_USER_ALIGN/TYPE_ALIGN values, but there is a lot of other code
that looks for type attributes by searching TYPE_ATTRIBUTES, not DECL_ATTRIBUTES
of TYPE_DECL (such as nonnul_arg_p in tree-vrp) or alloc_object_size.
Does it mean that those attributes are ignored for C++ produced types?

Honza
> 
> 	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 06:32:29PM +0200, Jan Hubicka wrote:
> > On Wed, Apr 08, 2015 at 06:22:10PM +0200, Jan Hubicka wrote:
> > > > On 04/08/2015 06:02 AM, Jakub Jelinek wrote:
> > > > >	(cp_build_qualified_type_real): Use check_base_type.  Build a
> > > > >	variant and copy over even TYPE_CONTEXT and
> > > > >	TYPE_ALIGN/TYPE_USER_ALIGN if any of those are different.
> > > > 
> > > > This seems wrong.  If there is an array with the same name,
> > > > attributes and element type, it should have the same alignment; if
> > > 
> > > One of problems is that cp_build_qualified_type rebuilds the array from
> > > scratch and never copies the attribute list around (as oposed to
> > > build_qualified_type that just memcpy the type node)
> > 
> > As I said earlier, TYPE_ATTRIBUTES is NULL here anyway, because the
> > attributes hang in DECL_ATTRIBUTES of TYPE_DECL.  And, except for
> > config/sol2.c (which looks wrong), nothing ever calls lookup_attribute for
> > "aligned" anyway, the user aligned stuff is encoded in TYPE_USER_ALIGN
> > and/or DECL_USER_ALIGN and TYPE_ALIGN/DECL_ALIGN.
> 
> This is interesting too.  I did know that alignment is "lowered" into
> TYPE_USER_ALIGN/TYPE_ALIGN values, but there is a lot of other code
> that looks for type attributes by searching TYPE_ATTRIBUTES, not DECL_ATTRIBUTES
> of TYPE_DECL (such as nonnul_arg_p in tree-vrp) or alloc_object_size.
> Does it mean that those attributes are ignored for C++ produced types?

I don't think this has anything to do with C++.  c-common.c has an attribute
table, for each attribute it has 3 flags, whether a decl is required, type
is required and/or fn type is required, and that determines to what the
attributes go.
These flags have the following combinations (decl/type/fntype # of attributes):
TFF 51 (i.e. decl required)
FTT 12 (i.e. function type required)
FTF 9  (i.e. some type required)
FFF 7  (applies to both types and decls)
Which means most of the attributes require a decl and thus go into
DECL_ATTRIBUTES, then some require function types and go to the
TYPE_ATTRIBUTES of the function, then others go solely to TYPE_ATTRIBUTES.
The last row are attributes that don't really care where they apply to, and
that is
  { "packed",                 0, 0, false, false, false,
  { "unused",                 0, 0, false, false, false,
  { "transparent_union",      0, 0, false, false, false,
  { "aligned",                0, 1, false, false, false,
  { "deprecated",             0, 1, false, false, false,
  { "visibility",	      1, 1, false, false, false,
  { "warn_unused",            0, 0, false, false, false,
where the first 6 really don't care about what is stored in
{TYPE,DECL}_ATTRIBUTES because the attributes are encoded differently in
generic, and the last one sounds like a mistake (perhaps one that can't be
undone anymore) where it doesn't require a type, but only stores it on types
and warns otherwise.

	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 12:02:39PM +0200, Jakub Jelinek wrote:
> The included (first) patch has been successfully bootstrapped/regtested on
> x86_64-linux and i686-linux, the attached patch not, but I can
> bootstrap/regtest it if you prefer it.

The attached version unfortunately regresses pr60226.c
typedef int __attribute__ ((aligned (1 << 28))) int28;
int28 foo[4] = {}; /* { dg-error "alignment of array elements is greater than element size" } */
error is not reported then.  But the first version with the layout_type call
in there works.

> @@ -1057,21 +1064,23 @@ cp_build_qualified_type_real (tree type,
>  	 should be equivalent to those in check_qualified_type.  */
>        for (t = TYPE_MAIN_VARIANT (type); t; t = TYPE_NEXT_VARIANT (t))
>  	if (TREE_TYPE (t) == element_type
> -	    && TYPE_NAME (t) == TYPE_NAME (type)
> -	    && TYPE_CONTEXT (t) == TYPE_CONTEXT (type)
> -	    && attribute_list_equal (TYPE_ATTRIBUTES (t),
> -				     TYPE_ATTRIBUTES (type)))
> +	    && check_base_type (t, type))

So, would instead of this change adding
  && TYPE_USER_ALIGN (t) == TYPE_USER_ALIGN (type)
  && (!TYPE_USER_ALIGN (t) || TYPE_ALIGN (t) == TYPE_ALIGN (type))
to the conditional work for you?
Or supposedly no change at all, as the attributes would be either in
TYPE_ATTRIBUTES, or on TYPE_DECL, but then it would be covered by
the TYPE_NAME comparison.

>  	  break;
>  
>        if (!t)
>  	{
>  	  t = build_cplus_array_type (element_type, TYPE_DOMAIN (type));
>  
> -	  /* Keep the typedef name.  */
> -	  if (TYPE_NAME (t) != TYPE_NAME (type))
> +	  /* Keep the typedef name, context and alignment.  */
> +	  if (TYPE_NAME (t) != TYPE_NAME (type)
> +	      || TYPE_CONTEXT (t) != TYPE_CONTEXT (type)
> +	      || TYPE_ALIGN (t) != TYPE_ALIGN (type))

Then supposedly similar change to the above one (the TYPE_USER_ALIGN stuff).

	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65690)

[Jason Merrill]

On 04/09/2015 05:38 AM, Jakub Jelinek wrote:
> Or supposedly no change at all, as the attributes would be either in
> TYPE_ATTRIBUTES, or on TYPE_DECL, but then it would be covered by
> the TYPE_NAME comparison.

Interesting point.  So maybe all we need to do here is copy 
TYPE_ALIGN/TYPE_USER_ALIGN, not change any of the tests.

Jason

[C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65715)

[Jakub Jelinek]

On Thu, Apr 09, 2015 at 10:51:12AM -0400, Jason Merrill wrote:
> On 04/09/2015 05:38 AM, Jakub Jelinek wrote:
> >Or supposedly no change at all, as the attributes would be either in
> >TYPE_ATTRIBUTES, or on TYPE_DECL, but then it would be covered by
> >the TYPE_NAME comparison.
> 
> Interesting point.  So maybe all we need to do here is copy
> TYPE_ALIGN/TYPE_USER_ALIGN, not change any of the tests.

The following is actually enough to fix this.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-04-09  Jakub Jelinek  <jakub@redhat.com>

	PR c++/65715
	* tree.c (cp_build_qualified_type_real): Also copy
	TYPE_ALIGN and TYPE_USER_ALIGN.

	* c-c++-common/attr-aligned-2.c: New test.

--- gcc/cp/tree.c.jj	2015-04-09 17:05:02.331837692 +0200
+++ gcc/cp/tree.c	2015-04-09 17:15:25.216896052 +0200
@@ -1079,6 +1079,8 @@ cp_build_qualified_type_real (tree type,
 	    {
 	      t = build_variant_type_copy (t);
 	      TYPE_NAME (t) = TYPE_NAME (type);
+	      TYPE_ALIGN (t) = TYPE_ALIGN (type);
+	      TYPE_USER_ALIGN (t) = TYPE_USER_ALIGN (type);
 	    }
 	}
 
--- gcc/testsuite/c-c++-common/attr-aligned-2.c.jj	2015-04-09 17:13:26.327792252 +0200
+++ gcc/testsuite/c-c++-common/attr-aligned-2.c	2015-04-09 17:13:57.054300285 +0200
@@ -0,0 +1,18 @@
+/* PR c++/65715 */
+/* { dg-do run } */
+
+typedef double T[4][4] __attribute__((aligned (2 * __alignof__ (double))));
+void foo (const T);
+struct S { T s; };
+
+int
+main ()
+{
+  if (__alignof__ (const T) != 2 * __alignof__ (double))
+    __builtin_abort ();
+  return 0;
+}
+
+#if defined(__cplusplus) && __cplusplus >= 201103L
+static_assert (alignof (const T) == 2 * alignof (double), "alignment of const T");
+#endif


	Jakub

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65715)

[Jason Merrill]

On 04/09/2015 02:12 PM, Jakub Jelinek wrote:
> The following is actually enough to fix this.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I actually already applied it, with your old combined testcase.

Jason

Re: [C++ PATCH] Fix alignment handling in build_cplus_array_type/cp_build_qualified_type_real (PR c++/65715)

[Jakub Jelinek]

On Fri, Apr 10, 2015 at 10:27:55AM -0400, Jason Merrill wrote:
> On 04/09/2015 02:12 PM, Jakub Jelinek wrote:
> >The following is actually enough to fix this.
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> I actually already applied it, with your old combined testcase.

Thanks.  And sorry for forgetting to commit the split testcase in my
original commit.

	Jakub