Patchwork [Dodji,Seketeli] Patch PR c++/45200

login
register
mail settings
Submitter Dodji Seketeli
Date Aug. 11, 2010, 6:25 p.m.
Message ID <m38w4dovkv.fsf@redhat.com>
Download mbox | patch
Permalink /patch/61497/
State New
Headers show

Comments

Dodji Seketeli - Aug. 11, 2010, 6:25 p.m.
Sorry, I previously forwarded this to the wrong mailing list.
Hello,

In the example accompanying the patch below we consider that the
types

     forward_as_lref<typenameseq::seq_type>

at line marked with //#0 and

     forward_as_lref<typename remove_reference<Seq>::type::seq_type>

at the line marked with //#1 should compare equal. And I believe that
is correct[1].

It follows that during the instantiantion of apply<reverse_view>,
lookup_class_template looks up an instantiation for
forward_as_lref<typename remove_reference<Seq>::type::seq_type>
and returns forward_as_lref<typenameseq::seq_type>. Then the
tsubst'ing of forward_as_lref<typenameseq::seq_type> in the
context of template<typename Seq> struct apply fails because it
tries to reuse the typedef seq that was defined in the
incompatible context template<typename Seq, typename N> struct
apply1.

This case seems to argue for stripping typedefs from the
TYPE_CONTEXT of TYPENAME_TYPEs when they are passed as template
arguments. I refrained from doing it before because I thought
that incompatible_dependent_types_p would be enough to handle all
comparisons involving dependent typedefs.

[1]: This is based on the resolution of PR c++/43800 at
http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01241.html

Tested on x86_64-unknown-linux-gnu against trunk and the 4.5
branch. OK to commit to both branches?

commit d9a0f93c1fda1d97cda5747003de84edd3812bda
Author: Dodji Seketeli <dodji@redhat.com>
Date:   Mon Aug 9 23:12:39 2010 +0200

    Fix PR c++/45200

    gcc/cp/Changelog:
    	PR c++/45200
    	* tree.c (strip_typedefs): Strip typedefs from the context of
    	TYPENAME_TYPEs.

    gcc/testsuite/ChangeLog:
    	PR c++/45200
    	* g++.dg/template/typedef34.C: New test.
Jason Merrill - Aug. 11, 2010, 9 p.m.
The change is OK, but the testcase looks over-reduced; apply1 isn't 
actually used at all.

Jason
Andrew Pinski - Aug. 11, 2010, 9:04 p.m.
On Wed, Aug 11, 2010 at 2:00 PM, Jason Merrill <jason@redhat.com> wrote:
> The change is OK, but the testcase looks over-reduced; apply1 isn't actually
> used at all.

Maybe I over reduced it but the ICE happened even if apply1 was used or not.

Thanks,
Andrew Pinski
Dodji Seketeli - Aug. 12, 2010, 1:16 p.m.
Jason Merrill <jason@redhat.com> writes:

> The change is OK, but the testcase looks over-reduced; apply1 isn't
> actually used at all.

It's true that apply1 is not used but it is useful to trigger the bug
because it has a different number of template parameters than struct
apply. E.g, removing apply1 or removing its last template parameter
makes the crash to vanish.
Jason Merrill - Aug. 12, 2010, 2:16 p.m.
On 08/12/2010 09:16 AM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:
>
>> The change is OK, but the testcase looks over-reduced; apply1 isn't
>> actually used at all.
>
> It's true that apply1 is not used but it is useful to trigger the bug
> because it has a different number of template parameters than struct
> apply. E.g, removing apply1 or removing its last template parameter
> makes the crash to vanish.

OK, then.

I'd like to see about moving back to using TYPE_CANONICAL to compare 
these types; it seems to me that might work if we just use different 
TYPE_CANONICAL for template parameters from parameter lists of different 
lengths.  So TYPE_CANONICAL would be the same for template type 
parameter 1 of 3 in all templates, but different from template type 
parameter 1 of 2.

Does that make sense to you?

Jason
Dodji Seketeli - Aug. 13, 2010, 10:08 a.m.
Jason Merrill <jason@redhat.com> writes:

> OK, then.
>
> I'd like to see about moving back to using TYPE_CANONICAL to compare
> these types; it seems to me that might work if we just use different
> TYPE_CANONICAL for template parameters from parameter lists of
> different lengths.  So TYPE_CANONICAL would be the same for template
> type parameter 1 of 3 in all templates, but different from template
> type parameter 1 of 2.
>
> Does that make sense to you?

I think that can work, yes. As I understand it the main benefit would be
to do away with the complex incompatible_dependent_types_p function.

Though, as we don't know the size of the full list of template parms
when we are building a given TEMPLATE_TYPE_PARM node I think we cannot
set its TYPE_CANONICAL at that moment, unlike how it is done now in
process_template_parm today. Rather, we would probably need to not set
the TYPE_CANONICAL at that moment, and and fix it up later when we know
the size of the list of template parsm, e.g. in end_template_parm_list.

That would let TYPE_CANONICAL in an incorrect state until the list of
template parms is parsed. Can't that lead to trouble?
Jason Merrill - Aug. 13, 2010, 3:01 p.m.
On 08/13/2010 06:08 AM, Dodji Seketeli wrote:
> That would let TYPE_CANONICAL in an incorrect state until the list of
> template parms is parsed. Can't that lead to trouble?

Yeah, when we're done with the list we'd want to rebuild anything 
elsewhere in the list that uses one of the parms, like the type of a 
non-type parameter.  Or try to determine how many parameters there are 
before parsing any of them.

Jason

Patch

diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 450b9e8..6b2aab0 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -1046,6 +1046,11 @@  strip_typedefs (tree t)
 					    TYPE_RAISES_EXCEPTIONS (t));
       }
       break;
+    case TYPENAME_TYPE:
+      result = make_typename_type (strip_typedefs (TYPE_CONTEXT (t)),
+				   TYPENAME_TYPE_FULLNAME (t),
+				   typename_type, tf_none);
+      break;
     default:
       break;
     }
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 484d299..a506053 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1212,7 +1212,7 @@  incompatible_dependent_types_p (tree t1, tree t2)
 
   if (!t1_typedef_variant_p || !t2_typedef_variant_p)
     /* Either T1 or T2 is not a typedef so we cannot compare the
-       the template parms of the typedefs of T1 and T2.
+       template parms of the typedefs of T1 and T2.
        At this point, if the main variant type of T1 and T2 are equal
        it means the two types can't be incompatible, from the perspective
        of this function.  */
diff --git a/gcc/testsuite/g++.dg/template/typedef34.C b/gcc/testsuite/g++.dg/template/typedef34.C
new file mode 100644
index 0000000..9bb4460
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/typedef34.C
@@ -0,0 +1,37 @@ 
+// Origin PR c++/45200
+// { dg-do compile }
+
+template<typename T>
+struct remove_reference
+{
+  typedef T type;
+};
+
+template<typename TestType>
+struct forward_as_lref
+{
+};
+
+template<typename Seq, typename N>
+struct apply1
+{
+  typedef typename remove_reference<Seq>::type seq;
+  typedef forward_as_lref<typename seq::seq_type> type; //#0
+};
+
+template<typename Seq>
+struct apply
+{
+  typedef forward_as_lref<typename remove_reference<Seq>::type::seq_type> type; //#1
+};
+
+struct reverse_view
+{
+  typedef int seq_type;
+};
+
+int
+main()
+{
+  apply<reverse_view >::type a2;
+}