[patch c++]: Fix for PR/65390

[patch c++]: Fix for PR/65390

[Kai Tietz]

Hi,

this patch avoids the attempt to create user-aligned-type for variants
original and main-variants type-alignment differ and original type
isn't user-aligned.
Not sure if this is the preferred variant, we could create for such
cases an aligned-type without setting user-align.
But as this should just happen for invalid types, I am not sure if we
actual need to handle later at all.
So I suggest the following fix for it:

ChangeLog

    PR c++/65390
    * tree.c (strip_typedefs): Don't attempt
    to build user-aligned type if original doesn't
    set it and main-variant differs to it.

Jonathan is just about to reduce the testcase, so final patch should
include reduced testcase for it.

Regression-tested for x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

Index: tree.c
===================================================================
--- tree.c      (Revision 221277)
+++ tree.c      (Arbeitskopie)
@@ -1356,7 +1356,7 @@ strip_typedefs (tree t)
   if (!result)
       result = TYPE_MAIN_VARIANT (t);
   if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
-      || TYPE_ALIGN (t) != TYPE_ALIGN (result))
+      || (TYPE_USER_ALIGN (t) && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
     {
       gcc_assert (TYPE_USER_ALIGN (t));
       if (TYPE_ALIGN (t) == TYPE_ALIGN (result))

Re: [patch c++]: Fix for PR/65390

[Jason Merrill]

If there is an alignment mismatch without user intervention, there is a 
problem, we can't just ignore it.

Where we run into trouble is with array types where the version built 
earlier has not been laid out yet but the new one has been.  I've been 
trying to deal with that by making sure that we lay out the original 
type as well, but obviously that isn't working for this case.  Why not?

I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN 
nor TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.

Jason

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
> If there is an alignment mismatch without user intervention, there is a
> problem, we can't just ignore it.
>
> Where we run into trouble is with array types where the version built
> earlier has not been laid out yet but the new one has been.  I've been
> trying to deal with that by making sure that we lay out the original type as
> well, but obviously that isn't working for this case.  Why not?

Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
(result) (value 8), and TYPE_USER_ALIGN isn't set.

> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN nor
> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.

For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
modified type).  So we could add here additional check if TYPE_SIZE is
a integer-constant?

Something like this condition you mean?

...
if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
    || ((TYPE_USER_ALIGN (t) || TREE_CODE (TYPE_SIZE (t)) == INTEGER_CST)
        && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
  {
...
> Jason

Kai

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

2015-03-16 20:22 GMT+01:00 Kai Tietz <ktietz70@googlemail.com>:
> 2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
>> If there is an alignment mismatch without user intervention, there is a
>> problem, we can't just ignore it.
>>
>> Where we run into trouble is with array types where the version built
>> earlier has not been laid out yet but the new one has been.  I've been
>> trying to deal with that by making sure that we lay out the original type as
>> well, but obviously that isn't working for this case.  Why not?
>
> Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
> (result) (value 8), and TYPE_USER_ALIGN isn't set.
>
>> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN nor
>> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.
>
> For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
> modified type).  So we could add here additional check if TYPE_SIZE is
> a integer-constant?
>
> Something like this condition you mean?

So tested following patch checking for existing TYPE_SIZE, plus if it
is a constant-value.

Index: tree.c
===================================================================
--- tree.c      (Revision 221277)
+++ tree.c      (Arbeitskopie)
@@ -1356,7 +1356,9 @@ strip_typedefs (tree t)
   if (!result)
       result = TYPE_MAIN_VARIANT (t);
   if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
-      || TYPE_ALIGN (t) != TYPE_ALIGN (result))
+      || ((TYPE_USER_ALIGN (t)
+          || (TYPE_SIZE (t) && TREE_CODE (TYPE_SIZE (t)) == INTEGER_CST))
+         && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
     {
       gcc_assert (TYPE_USER_ALIGN (t));
       if (TYPE_ALIGN (t) == TYPE_ALIGN (result))

Re: [patch c++]: Fix for PR/65390

[Jason Merrill]

On 03/16/2015 03:22 PM, Kai Tietz wrote:
> 2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
>> If there is an alignment mismatch without user intervention, there is a
>> problem, we can't just ignore it.
>>
>> Where we run into trouble is with array types where the version built
>> earlier has not been laid out yet but the new one has been.  I've been
>> trying to deal with that by making sure that we lay out the original type as
>> well, but obviously that isn't working for this case.  Why not?
>
> Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
> (result) (value 8), and TYPE_USER_ALIGN isn't set.
>
>> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN nor
>> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.
>
> For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
> modified type).

TYPE_ALIGN should still be correct in that case.  So we need to figure 
out why result is getting the wrong alignment.

Jason

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

2015-03-17 13:36 GMT+01:00 Jason Merrill <jason@redhat.com>:
> On 03/16/2015 03:22 PM, Kai Tietz wrote:
>>
>> 2015-03-16 19:07 GMT+01:00 Jason Merrill <jason@redhat.com>:
>>>
>>> If there is an alignment mismatch without user intervention, there is a
>>> problem, we can't just ignore it.
>>>
>>> Where we run into trouble is with array types where the version built
>>> earlier has not been laid out yet but the new one has been.  I've been
>>> trying to deal with that by making sure that we lay out the original type
>>> as
>>> well, but obviously that isn't working for this case.  Why not?
>>
>>
>> Well, TYPE_ALIGN (t) is set to 32, and it differs to TYPE_ALIGN
>> (result) (value 8), and TYPE_USER_ALIGN isn't set.
>>
>>> I suppose we could avoid checking TYPE_ALIGN if neither TYPE_USER_ALIGN
>>> nor
>>> TYPE_SIZE are set on 't', but checking TYPE_USER_ALIGN isn't enough.
>>
>>
>> For t TYPE_SIZE is set, but it isn't a constant (as it is an variably
>> modified type).
>
>
> TYPE_ALIGN should still be correct in that case.  So we need to figure out
> why result is getting the wrong alignment.
>
> Jason
>

By debugging in build_cplus_array_type I see that type is marked as
dependent.  This is caused by type-max being an expression
non-constant.  So we later on don't layout this type.
So result isn't a comlete layout type.  by callling layout_type on
result, alignment fits.

Kai

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

Hello,

the problem here is that for cases of vla-array-types, the types don't
get finally layouted in build_cplus_array_type.  So the type-alignment
isn't set in such cases for the resulting type.

ChangeLog

2015-03-20  Kai Tietz  <ktietz@redhat.com>

    PR c++/65390
    * tree.c (strip_typedefs): Ignore alignment
    difference during processing template.

2015-03-20  Kai Tietz  <ktietz@redhat.com>

    PR c++/65390
    * g++.dg/template/pr65390.C: New file.

Tested on x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

Index: gcc/gcc/cp/tree.c
===================================================================
--- gcc.orig/gcc/cp/tree.c
+++ gcc/gcc/cp/tree.c
@@ -1356,7 +1356,8 @@ strip_typedefs (tree t)
   if (!result)
       result = TYPE_MAIN_VARIANT (t);
   if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
-      || TYPE_ALIGN (t) != TYPE_ALIGN (result))
+      || (processing_template_decl
+      && TYPE_ALIGN (t) != TYPE_ALIGN (result)))
     {
       gcc_assert (TYPE_USER_ALIGN (t));
       if (TYPE_ALIGN (t) == TYPE_ALIGN (result))
Index: gcc/gcc/testsuite/g++.dg/template/pr65390.C
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/g++.dg/template/pr65390.C
@@ -0,0 +1,12 @@
+// { dg-do compile }
+// { dg-options "-Wno-vla" }
+template<typename T> struct shared_ptr { };
+
+template<typename T, typename Arg>
+shared_ptr<T> make_shared(Arg) { return shared_ptr<T>(); } // {
dg-message "note" }
+// { dg-warning "ignoring attributes" "template" { target *-*-* } 6 }
+
+void f(int n){
+  make_shared<int[n]>(1); // { dg-error "no matching" }
+}
+// { dg-error "variably modified type|trying to instantiate" "type" {
target *-*-* } 10 }

Re: [patch c++]: Fix for PR/65390

[Jason Merrill]

On 03/20/2015 10:53 AM, Kai Tietz wrote:
>      * tree.c (strip_typedefs): Ignore alignment
>      difference during processing template.
>
> +      || (processing_template_decl
> +      && TYPE_ALIGN (t) != TYPE_ALIGN (result)))

Your change is actually ignoring alignment differences when *not* 
processing a template, which isn't what we want.

The problem is that the type is considered dependent in a template but 
is not actually dependent, so we can see the exact same type outside a 
template and it's not dependent.  So, this code is creating the difference:

>   /* We can only call value_dependent_expression_p on integral constant
>      expressions; treat non-constant expressions as dependent, too.  */
>   if (processing_template_decl
>       && (type_dependent_expression_p (size)
>           || !TREE_CONSTANT (size) || value_dependent_expression_p (size)))

Now that we have instantiation_dependent_expression_p, we should be able 
to use that instead of checking type/value dependency separately.

Jason

Re: [patch c++]: Fix for PR/65390

[Marek Polacek]

On Tue, Mar 24, 2015 at 01:14:50AM -0400, Jason Merrill wrote:

Here's my shot at this.

> The problem is that the type is considered dependent in a template but is
> not actually dependent, so we can see the exact same type outside a template

Yeah, I think this is true...

> and it's not dependent.  So, this code is creating the difference:
 
> >  /* We can only call value_dependent_expression_p on integral constant
> >     expressions; treat non-constant expressions as dependent, too.  */
> >  if (processing_template_decl
> >      && (type_dependent_expression_p (size)
> >          || !TREE_CONSTANT (size) || value_dependent_expression_p (size)))
> 
> Now that we have instantiation_dependent_expression_p, we should be able to
> use that instead of checking type/value dependency separately.

...but I think there's another place where things go wrong.  ISTM that in
build_cplus_array_type we consider all arrays with non-constant index as
dependent (when processing_template_decl) -- but as the testcase shows, this
is not always true.  The fix then could look like the following, though I
wouldn't be surprised if this was a wrong way how to go about this.

Bootstrapped/regtested on x86_64-linux.  Not a regression, so we might want to
defer this patch to the next stage1.

2015-03-31  Marek Polacek  <polacek@redhat.com>

	PR c++/65390
	* tree.c (build_cplus_array_type): Use dependent_type_p rather than
	checking for constness.

	* g++.dg/template/pr65390.C: New test.

diff --git gcc/cp/tree.c gcc/cp/tree.c
index ef53aff..97bccc0 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -822,10 +822,9 @@ build_cplus_array_type (tree elt_type, tree index_type)
   if (elt_type == error_mark_node || index_type == error_mark_node)
     return error_mark_node;
 
-  bool dependent
-    = (processing_template_decl
-       && (dependent_type_p (elt_type)
-	   || (index_type && !TREE_CONSTANT (TYPE_MAX_VALUE (index_type)))));
+  bool dependent = (processing_template_decl
+		    && (dependent_type_p (elt_type)
+			|| (index_type && dependent_type_p (index_type))));
 
   if (elt_type != TYPE_MAIN_VARIANT (elt_type))
     /* Start with an array of the TYPE_MAIN_VARIANT.  */
diff --git gcc/testsuite/g++.dg/template/pr65390.C gcc/testsuite/g++.dg/template/pr65390.C
index e69de29..299d22a 100644
--- gcc/testsuite/g++.dg/template/pr65390.C
+++ gcc/testsuite/g++.dg/template/pr65390.C
@@ -0,0 +1,12 @@
+// PR c++/65390
+// { dg-do compile }
+// { dg-options "" }
+
+template<typename T> struct shared_ptr { };
+
+template<typename T, typename Arg>
+shared_ptr<T> make_shared(Arg) { return shared_ptr<T>(); } // { dg-error "variably modified type|trying to instantiate" }
+
+void f(int n){
+  make_shared<int[n]>(1); // { dg-error "no matching function" }
+}

	Marek

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

Hi,

I had tried same approach as Marek.  For me it solved the PR, but
caused other regressions on boostrap.  So I dropped the way via
dependent_type_p.

Well, this bootstrap-issue might be caused by some local changes I had
forgot to remove, but I doubt it.
Marek, have you tried to do a boostrap with your patch?

Kai

2015-03-31 13:50 GMT+02:00 Marek Polacek <polacek@redhat.com>:
> On Tue, Mar 24, 2015 at 01:14:50AM -0400, Jason Merrill wrote:
>
> Here's my shot at this.
>
>> The problem is that the type is considered dependent in a template but is
>> not actually dependent, so we can see the exact same type outside a template
>
> Yeah, I think this is true...
>
>> and it's not dependent.  So, this code is creating the difference:
>
>> >  /* We can only call value_dependent_expression_p on integral constant
>> >     expressions; treat non-constant expressions as dependent, too.  */
>> >  if (processing_template_decl
>> >      && (type_dependent_expression_p (size)
>> >          || !TREE_CONSTANT (size) || value_dependent_expression_p (size)))
>>
>> Now that we have instantiation_dependent_expression_p, we should be able to
>> use that instead of checking type/value dependency separately.
>
> ...but I think there's another place where things go wrong.  ISTM that in
> build_cplus_array_type we consider all arrays with non-constant index as
> dependent (when processing_template_decl) -- but as the testcase shows, this
> is not always true.  The fix then could look like the following, though I
> wouldn't be surprised if this was a wrong way how to go about this.
>
> Bootstrapped/regtested on x86_64-linux.  Not a regression, so we might want to
> defer this patch to the next stage1.
>
> 2015-03-31  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/65390
>         * tree.c (build_cplus_array_type): Use dependent_type_p rather than
>         checking for constness.
>
>         * g++.dg/template/pr65390.C: New test.
>
> diff --git gcc/cp/tree.c gcc/cp/tree.c
> index ef53aff..97bccc0 100644
> --- gcc/cp/tree.c
> +++ gcc/cp/tree.c
> @@ -822,10 +822,9 @@ build_cplus_array_type (tree elt_type, tree index_type)
>    if (elt_type == error_mark_node || index_type == error_mark_node)
>      return error_mark_node;
>
> -  bool dependent
> -    = (processing_template_decl
> -       && (dependent_type_p (elt_type)
> -          || (index_type && !TREE_CONSTANT (TYPE_MAX_VALUE (index_type)))));
> +  bool dependent = (processing_template_decl
> +                   && (dependent_type_p (elt_type)
> +                       || (index_type && dependent_type_p (index_type))));
>
>    if (elt_type != TYPE_MAIN_VARIANT (elt_type))
>      /* Start with an array of the TYPE_MAIN_VARIANT.  */
> diff --git gcc/testsuite/g++.dg/template/pr65390.C gcc/testsuite/g++.dg/template/pr65390.C
> index e69de29..299d22a 100644
> --- gcc/testsuite/g++.dg/template/pr65390.C
> +++ gcc/testsuite/g++.dg/template/pr65390.C
> @@ -0,0 +1,12 @@
> +// PR c++/65390
> +// { dg-do compile }
> +// { dg-options "" }
> +
> +template<typename T> struct shared_ptr { };
> +
> +template<typename T, typename Arg>
> +shared_ptr<T> make_shared(Arg) { return shared_ptr<T>(); } // { dg-error "variably modified type|trying to instantiate" }
> +
> +void f(int n){
> +  make_shared<int[n]>(1); // { dg-error "no matching function" }
> +}
>
>         Marek

Re: [patch c++]: Fix for PR/65390

[Marek Polacek]

On Tue, Mar 31, 2015 at 02:25:14PM +0200, Kai Tietz wrote:
> Hi,
> 
> I had tried same approach as Marek.  For me it solved the PR, but
> caused other regressions on boostrap.  So I dropped the way via
> dependent_type_p.
> 
> Well, this bootstrap-issue might be caused by some local changes I had
> forgot to remove, but I doubt it.
> Marek, have you tried to do a boostrap with your patch?

Of course, with --enable-languages=all.  I'll re-run the bootstrap with more
languages enabled, though.

	Marek

Re: [patch c++]: Fix for PR/65390

[Marek Polacek]

On Tue, Mar 31, 2015 at 02:32:32PM +0200, Marek Polacek wrote:
> On Tue, Mar 31, 2015 at 02:25:14PM +0200, Kai Tietz wrote:
> > Hi,
> > 
> > I had tried same approach as Marek.  For me it solved the PR, but
> > caused other regressions on boostrap.  So I dropped the way via
> > dependent_type_p.
> > 
> > Well, this bootstrap-issue might be caused by some local changes I had
> > forgot to remove, but I doubt it.
> > Marek, have you tried to do a boostrap with your patch?
> 
> Of course, with --enable-languages=all.  I'll re-run the bootstrap with more
> languages enabled, though.

BTW, are you saying that your fix was exactly the same?  Did you as well check
that index_type is non-null?

	Marek

Re: [patch c++]: Fix for PR/65390

[Kai Tietz]

2015-03-31 14:34 GMT+02:00 Marek Polacek <polacek@redhat.com>:
> On Tue, Mar 31, 2015 at 02:32:32PM +0200, Marek Polacek wrote:
>> On Tue, Mar 31, 2015 at 02:25:14PM +0200, Kai Tietz wrote:
>> > Hi,
>> >
>> > I had tried same approach as Marek.  For me it solved the PR, but
>> > caused other regressions on boostrap.  So I dropped the way via
>> > dependent_type_p.
>> >
>> > Well, this bootstrap-issue might be caused by some local changes I had
>> > forgot to remove, but I doubt it.
>> > Marek, have you tried to do a boostrap with your patch?
>>
>> Of course, with --enable-languages=all.  I'll re-run the bootstrap with more
>> languages enabled, though.
>
> BTW, are you saying that your fix was exactly the same?  Did you as well check
> that index_type is non-null?

Sure, I checked for index_type.  But by looking closer I used
instantiation_dependent_expression_p - as mentioned by Jason - instead
of dependent_type_p, which seems to make here the difference.

>         Marek

Kai

Re: [patch c++]: Fix for PR/65390

[Marek Polacek]

On Tue, Mar 31, 2015 at 02:32:32PM +0200, Marek Polacek wrote:
> Of course, with --enable-languages=all.  I'll re-run the bootstrap with more
> languages enabled, though.

--enable-languages=all,obj-c++,go bootstrap passed again on x86_64 and ppc64.

	Marek

Re: [patch c++]: Fix for PR/65390

[Jason Merrill]

OK, thanks.

Jason