Patchwork [C++] Fix up strip_typedefs (PR debug/56819)

login
register
mail settings
Submitter Jakub Jelinek
Date April 3, 2013, 12:36 p.m.
Message ID <20130403123600.GC4201@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/233430/
State New
Headers show

Comments

Jakub Jelinek - April 3, 2013, 12:36 p.m.
Hi!

On the following testcase we ICE with -fcompare-debug with
--enable-checking=yes, because strip_typedefs copies args to a new TREE_VEC,
but doesn't copy over NON_DEFAULT_TEMPLATE_ARGS_COUNT.  For ENABLE_CHECKING
the code requires that it is set, for !ENABLE_CHECKING it would be needed
only if NON_DEFAULT_TEMPLATE_ARGS_COUNT was already non-NULL (otherwise
GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT would use length of the TREE_VEC).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?

2013-04-03  Jakub Jelinek  <jakub@redhat.com>

	PR debug/56819
	* tree.c (strip_typedefs): SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
	on new_args.

	* g++.dg/debug/pr56819.C: New test.


	Jakub
Gabriel Dos Reis - April 3, 2013, 1:38 p.m.
On Wed, Apr 3, 2013 at 7:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the following testcase we ICE with -fcompare-debug with
> --enable-checking=yes, because strip_typedefs copies args to a new TREE_VEC,
> but doesn't copy over NON_DEFAULT_TEMPLATE_ARGS_COUNT.  For ENABLE_CHECKING
> the code requires that it is set, for !ENABLE_CHECKING it would be needed
> only if NON_DEFAULT_TEMPLATE_ARGS_COUNT was already non-NULL (otherwise
> GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT would use length of the TREE_VEC).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?
>
> 2013-04-03  Jakub Jelinek  <jakub@redhat.com>
>
>         PR debug/56819
>         * tree.c (strip_typedefs): SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
>         on new_args.
>
>         * g++.dg/debug/pr56819.C: New test.
>
> --- gcc/cp/tree.c.jj    2013-04-02 20:24:34.000000000 +0200
> +++ gcc/cp/tree.c       2013-04-03 10:51:56.614548181 +0200
> @@ -1255,8 +1255,16 @@ strip_typedefs (tree t)
>                   changed = true;
>               }
>             if (changed)
> -             fullname = lookup_template_function (TREE_OPERAND (fullname, 0),
> -                                                  new_args);
> +             {
> +#ifndef ENABLE_CHECKING
> +               if (NON_DEFAULT_TEMPLATE_ARGS_COUNT (args))
> +#endif
> +                 SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
> +                   (new_args, GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (args));
> +               fullname
> +                 = lookup_template_function (TREE_OPERAND (fullname, 0),
> +                                             new_args);
> +             }
>             else
>               ggc_free (new_args);

Hmm, the resulting code does not look simpler.
Why can't we always copy the stuff as opposed to
playing cat-n-fish with the CPP macro ENABLE_CHECKING?

-- Gaby
Jason Merrill - April 3, 2013, 1:43 p.m.
We should be able to just copy NON_DEFAULT_TEMPLATE_ARGS_COUNT over 
rather than mess with looking into and building an INTEGER_CST.

Jason
Jakub Jelinek - April 3, 2013, 1:45 p.m.
On Wed, Apr 03, 2013 at 08:38:01AM -0500, Gabriel Dos Reis wrote:
> > 2013-04-03  Jakub Jelinek  <jakub@redhat.com>
> >
> >         PR debug/56819
> >         * tree.c (strip_typedefs): SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
> >         on new_args.
> >
> >         * g++.dg/debug/pr56819.C: New test.
> >
> > --- gcc/cp/tree.c.jj    2013-04-02 20:24:34.000000000 +0200
> > +++ gcc/cp/tree.c       2013-04-03 10:51:56.614548181 +0200
> > @@ -1255,8 +1255,16 @@ strip_typedefs (tree t)
> >                   changed = true;
> >               }
> >             if (changed)
> > -             fullname = lookup_template_function (TREE_OPERAND (fullname, 0),
> > -                                                  new_args);
> > +             {
> > +#ifndef ENABLE_CHECKING
> > +               if (NON_DEFAULT_TEMPLATE_ARGS_COUNT (args))
> > +#endif
> > +                 SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
> > +                   (new_args, GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (args));
> > +               fullname
> > +                 = lookup_template_function (TREE_OPERAND (fullname, 0),
> > +                                             new_args);
> > +             }
> >             else
> >               ggc_free (new_args);
> 
> Hmm, the resulting code does not look simpler.
> Why can't we always copy the stuff as opposed to
> playing cat-n-fish with the CPP macro ENABLE_CHECKING?

I didn't want to slow it down in the common case (--enable-checking=release)
and null NON_DEFAULT_TEMPLATE_ARGS_COUNT, in that case it would
build_int_cst unnecessarily.  If that isn't something we care about, why are
we differentiating between ENABLE_CHECKING vs. !ENABLE_CHECKING for
*NON_DEFAULT_TEMPLATE_ARGS_COUNT everywhere at all?
Though, as INTEGER_CSTs should be shared, perhaps this could be just
  NON_DEFAULT_TEMPLATE_ARGS_COUNT (new_args)
    = NON_DEFAULT_TEMPLATE_ARGS_COUNT (args);
and strip_typedefs_expr could be changed to do the same thing.

	Jakub
Gabriel Dos Reis - April 3, 2013, 3:21 p.m.
> Though, as INTEGER_CSTs should be shared, perhaps this could be just
>   NON_DEFAULT_TEMPLATE_ARGS_COUNT (new_args)
>     = NON_DEFAULT_TEMPLATE_ARGS_COUNT (args);

Yes, thanks!

> and strip_typedefs_expr could be changed to do the same thing.

Patch

--- gcc/cp/tree.c.jj	2013-04-02 20:24:34.000000000 +0200
+++ gcc/cp/tree.c	2013-04-03 10:51:56.614548181 +0200
@@ -1255,8 +1255,16 @@  strip_typedefs (tree t)
 		  changed = true;
 	      }
 	    if (changed)
-	      fullname = lookup_template_function (TREE_OPERAND (fullname, 0),
-						   new_args);
+	      {
+#ifndef ENABLE_CHECKING
+		if (NON_DEFAULT_TEMPLATE_ARGS_COUNT (args))
+#endif
+		  SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT
+		    (new_args, GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (args));
+		fullname
+		  = lookup_template_function (TREE_OPERAND (fullname, 0),
+					      new_args);
+	      }
 	    else
 	      ggc_free (new_args);
 	  }
--- gcc/testsuite/g++.dg/debug/pr56819.C.jj	2013-04-03 10:57:55.187410867 +0200
+++ gcc/testsuite/g++.dg/debug/pr56819.C	2013-04-03 10:56:42.000000000 +0200
@@ -0,0 +1,27 @@ 
+// PR debug/56819
+// { dg-do compile }
+// { dg-options "-fcompare-debug" }
+
+template <typename>
+struct A
+{
+  template <typename>
+  struct B;
+};
+
+template <typename>
+struct C
+{
+  typedef int I;
+};
+
+template <typename T>
+class D
+{
+  typedef A <void> E;
+  typedef typename T::template B <E> F;
+  typedef typename C <F>::I I;
+  A <I> foo () { return A<I> (); }
+};
+
+template class D <A <void> >;