Fix ICE in tinst_level refcounting introduced in r259457 (PR c++/85462)

Message ID 20180419182721.GA8577@tucnak
State New
Headers show
Series
  • Fix ICE in tinst_level refcounting introduced in r259457 (PR c++/85462)
Related show

Commit Message

Jakub Jelinek April 19, 2018, 6:27 p.m.
Hi!

The following testcase ICEs starting with r259457 PR80290 fix.

The problem is that the refcount is just 8-bit and if we need more than 256
refcounts for one tinst_level, we fail an assertion.

As discovered by Richard, the in_system_header_p member is write-only
since r138031:
grep in_system_header_p cp/*.[ch]
cp/cp-tree.h:  bool in_system_header_p;
cp/pt.c:  new_level->in_system_header_p = in_system_header_at (input_location);
so we can easily even without using bitfields enlarge the refcount to
16-bits and make it far less likely to hit the assertion.

This patch implements also saturation, once the refcount goes to 65535, it
becomes immutable and we then never return the tinst_level into the free
list.  It doesn't matter that much, because it is still GC allocated and can
be freed during ggc_collect if nothing refers to it.

I've tested the patch on the testcase also with 8-bit refcount and 255 as
rlimit_infinity.

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

2018-04-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/85462
	* cp-tree.h (tinst_level): Remove in_system_header_p member,
	change refcount member from unsigned char to unsigned short,
	add refcount_infinity static data member, adjust comments.
	* pt.c (tinst_level::refcount_infinity): Define.
	(inc_refcount_use): Remove assert, don't increment if refcount
	is already refcount_infinity, adjust comment.
	(dec_refcount_use): Remove assert, don't decrement if refcount
	is refcount_infinity, adjust comment.
	(push_tinst_level_loc): Formatting fix.

	* g++.dg/cpp0x/pr85462.C: New test.


	Jakub

Comments

Jason Merrill April 19, 2018, 11:43 p.m. | #1
Ok.

On Thu, Apr 19, 2018, 12:27 PM Jakub Jelinek <jakub@redhat.com> wrote:

> Hi!

>

> The following testcase ICEs starting with r259457 PR80290 fix.

>

> The problem is that the refcount is just 8-bit and if we need more than 256

> refcounts for one tinst_level, we fail an assertion.

>

> As discovered by Richard, the in_system_header_p member is write-only

> since r138031:

> grep in_system_header_p cp/*.[ch]

> cp/cp-tree.h:  bool in_system_header_p;

> cp/pt.c:  new_level->in_system_header_p = in_system_header_at

> (input_location);

> so we can easily even without using bitfields enlarge the refcount to

> 16-bits and make it far less likely to hit the assertion.

>

> This patch implements also saturation, once the refcount goes to 65535, it

> becomes immutable and we then never return the tinst_level into the free

> list.  It doesn't matter that much, because it is still GC allocated and

> can

> be freed during ggc_collect if nothing refers to it.

>

> I've tested the patch on the testcase also with 8-bit refcount and 255 as

> rlimit_infinity.

>

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

>

> 2018-04-19  Jakub Jelinek  <jakub@redhat.com>

>

>         PR c++/85462

>         * cp-tree.h (tinst_level): Remove in_system_header_p member,

>         change refcount member from unsigned char to unsigned short,

>         add refcount_infinity static data member, adjust comments.

>         * pt.c (tinst_level::refcount_infinity): Define.

>         (inc_refcount_use): Remove assert, don't increment if refcount

>         is already refcount_infinity, adjust comment.

>         (dec_refcount_use): Remove assert, don't decrement if refcount

>         is refcount_infinity, adjust comment.

>         (push_tinst_level_loc): Formatting fix.

>

>         * g++.dg/cpp0x/pr85462.C: New test.

>

> --- gcc/cp/cp-tree.h.jj 2018-04-19 09:42:44.409864659 +0200

> +++ gcc/cp/cp-tree.h    2018-04-19 12:33:25.339085905 +0200

> @@ -5927,14 +5927,19 @@ struct GTY((chain_next ("%h.next"))) tin

>    /* The location where the template is instantiated.  */

>    location_t locus;

>

> -  /* errorcount+sorrycount when we pushed this level.  */

> +  /* errorcount + sorrycount when we pushed this level.  */

>    unsigned short errors;

>

> -  /* True if the location is in a system header.  */

> -  bool in_system_header_p;

> +  /* Count references to this object.  If refcount reaches

> +     refcount_infinity value, we don't increment or decrement the

> +     refcount anymore, as the refcount isn't accurate anymore.

> +     The object can be still garbage collected if unreferenced from

> +     anywhere, which might keep referenced objects referenced longer than

> +     otherwise necessary.  Hitting the infinity is rare though.  */

> +  unsigned short refcount;

>

> -  /* Count references to this object.  */

> -  unsigned char refcount;

> +  /* Infinity value for the above refcount.  */

> +  static const unsigned short refcount_infinity = (unsigned short) ~0;

>  };

>

>  bool decl_spec_seq_has_spec_p (const cp_decl_specifier_seq *,

> cp_decl_spec);

> --- gcc/cp/pt.c.jj      2018-04-18 10:45:22.901720592 +0200

> +++ gcc/cp/pt.c 2018-04-19 12:33:17.704080557 +0200

> @@ -8945,15 +8945,14 @@ tinst_level::to_list ()

>    return ret;

>  }

>

> -/* Increment OBJ's refcount.  */

> +const unsigned short tinst_level::refcount_infinity;

> +

> +/* Increment OBJ's refcount unless it is already infinite.  */

>  static tinst_level *

>  inc_refcount_use (tinst_level *obj)

>  {

> -  if (obj)

> -    {

> -      ++obj->refcount;

> -      gcc_assert (obj->refcount != 0);

> -    }

> +  if (obj && obj->refcount != tinst_level::refcount_infinity)

> +    ++obj->refcount;

>    return obj;

>  }

>

> @@ -8966,15 +8965,16 @@ tinst_level::free (tinst_level *obj)

>    tinst_level_freelist ().free (obj);

>  }

>

> -/* Decrement OBJ's refcount.  If it reaches zero, release OBJ's DECL

> -   and OBJ, and start over with the tinst_level object that used to be

> -   referenced by OBJ's NEXT.  */

> +/* Decrement OBJ's refcount if not infinite.  If it reaches zero, release

> +   OBJ's DECL and OBJ, and start over with the tinst_level object that

> +   used to be referenced by OBJ's NEXT.  */

>  static void

>  dec_refcount_use (tinst_level *obj)

>  {

> -  while (obj && !--obj->refcount)

> +  while (obj

> +        && obj->refcount != tinst_level::refcount_infinity

> +        && !--obj->refcount)

>      {

> -      gcc_assert (obj->refcount+1 != 0);

>        tinst_level *next = obj->next;

>        tinst_level::free (obj);

>        obj = next;

> @@ -10143,8 +10143,7 @@ push_tinst_level_loc (tree tldcl, tree t

>    new_level->tldcl = tldcl;

>    new_level->targs = targs;

>    new_level->locus = loc;

> -  new_level->errors = errorcount+sorrycount;

> -  new_level->in_system_header_p = in_system_header_at (input_location);

> +  new_level->errors = errorcount + sorrycount;

>    new_level->next = NULL;

>    new_level->refcount = 0;

>    set_refcount_ptr (new_level->next, current_tinst_level);

> --- gcc/testsuite/g++.dg/cpp0x/pr85462.C.jj     2018-04-19

> 19:16:50.994498502 +0200

> +++ gcc/testsuite/g++.dg/cpp0x/pr85462.C        2018-04-19

> 19:20:38.961594082 +0200

> @@ -0,0 +1,38 @@

> +// PR c++/85462

> +// { dg-do compile { target c++11 } }

> +

> +template <class T> struct D { using d = T *; };

> +template <class, class, class> struct E;

> +template <class T, class U> struct E<T, U, U> { using d = typename

> D<T>::d; };

> +template <class T> struct G { using d = typename E<T, int, int>::d; };

> +template <class T, class U> typename G<T>::d foo (U);

> +#define A(n) class A##n {};

> +#define B(n) A(n##0) A(n##1) A(n##2) A(n##3) A(n##4) A(n##5) A(n##6)

> A(n##7) A(n##8) A(n##9)

> +#define C(n) B(n##0) B(n##1) B(n##2) B(n##3) B(n##4) B(n##5) B(n##6)

> B(n##7) B(n##8) B(n##9)

> +#define D(n) C(n##0) C(n##1) C(n##2) C(n##3) C(n##4)

> +D(1)

> +class H;

> +template <typename>

> +struct I

> +{

> +  bool bar ();

> +#undef A

> +#define A(n) void f##n (A##n *);

> +D(1)

> +  void baz ();

> +};

> +A1000 v;

> +template <typename T>

> +bool I<T>::bar ()

> +{

> +#undef A

> +#define A(n) A##n k##n = *foo<A##n> (v); f##n (&k##n);

> +D(1)

> +  foo<H> (v);

> +  baz ();

> +  return false;

> +}

> +struct J : I<int>

> +{

> +  void qux () { bar (); }

> +};

>

>         Jakub

>

Patch

--- gcc/cp/cp-tree.h.jj	2018-04-19 09:42:44.409864659 +0200
+++ gcc/cp/cp-tree.h	2018-04-19 12:33:25.339085905 +0200
@@ -5927,14 +5927,19 @@  struct GTY((chain_next ("%h.next"))) tin
   /* The location where the template is instantiated.  */
   location_t locus;
 
-  /* errorcount+sorrycount when we pushed this level.  */
+  /* errorcount + sorrycount when we pushed this level.  */
   unsigned short errors;
 
-  /* True if the location is in a system header.  */
-  bool in_system_header_p;
+  /* Count references to this object.  If refcount reaches
+     refcount_infinity value, we don't increment or decrement the
+     refcount anymore, as the refcount isn't accurate anymore.
+     The object can be still garbage collected if unreferenced from
+     anywhere, which might keep referenced objects referenced longer than
+     otherwise necessary.  Hitting the infinity is rare though.  */
+  unsigned short refcount;
 
-  /* Count references to this object.  */
-  unsigned char refcount;
+  /* Infinity value for the above refcount.  */
+  static const unsigned short refcount_infinity = (unsigned short) ~0;
 };
 
 bool decl_spec_seq_has_spec_p (const cp_decl_specifier_seq *, cp_decl_spec);
--- gcc/cp/pt.c.jj	2018-04-18 10:45:22.901720592 +0200
+++ gcc/cp/pt.c	2018-04-19 12:33:17.704080557 +0200
@@ -8945,15 +8945,14 @@  tinst_level::to_list ()
   return ret;
 }
 
-/* Increment OBJ's refcount.  */
+const unsigned short tinst_level::refcount_infinity;
+
+/* Increment OBJ's refcount unless it is already infinite.  */
 static tinst_level *
 inc_refcount_use (tinst_level *obj)
 {
-  if (obj)
-    {
-      ++obj->refcount;
-      gcc_assert (obj->refcount != 0);
-    }
+  if (obj && obj->refcount != tinst_level::refcount_infinity)
+    ++obj->refcount;
   return obj;
 }
 
@@ -8966,15 +8965,16 @@  tinst_level::free (tinst_level *obj)
   tinst_level_freelist ().free (obj);
 }
 
-/* Decrement OBJ's refcount.  If it reaches zero, release OBJ's DECL
-   and OBJ, and start over with the tinst_level object that used to be
-   referenced by OBJ's NEXT.  */
+/* Decrement OBJ's refcount if not infinite.  If it reaches zero, release
+   OBJ's DECL and OBJ, and start over with the tinst_level object that
+   used to be referenced by OBJ's NEXT.  */
 static void
 dec_refcount_use (tinst_level *obj)
 {
-  while (obj && !--obj->refcount)
+  while (obj
+	 && obj->refcount != tinst_level::refcount_infinity
+	 && !--obj->refcount)
     {
-      gcc_assert (obj->refcount+1 != 0);
       tinst_level *next = obj->next;
       tinst_level::free (obj);
       obj = next;
@@ -10143,8 +10143,7 @@  push_tinst_level_loc (tree tldcl, tree t
   new_level->tldcl = tldcl;
   new_level->targs = targs;
   new_level->locus = loc;
-  new_level->errors = errorcount+sorrycount;
-  new_level->in_system_header_p = in_system_header_at (input_location);
+  new_level->errors = errorcount + sorrycount;
   new_level->next = NULL;
   new_level->refcount = 0;
   set_refcount_ptr (new_level->next, current_tinst_level);
--- gcc/testsuite/g++.dg/cpp0x/pr85462.C.jj	2018-04-19 19:16:50.994498502 +0200
+++ gcc/testsuite/g++.dg/cpp0x/pr85462.C	2018-04-19 19:20:38.961594082 +0200
@@ -0,0 +1,38 @@ 
+// PR c++/85462
+// { dg-do compile { target c++11 } }
+
+template <class T> struct D { using d = T *; };
+template <class, class, class> struct E;
+template <class T, class U> struct E<T, U, U> { using d = typename D<T>::d; };
+template <class T> struct G { using d = typename E<T, int, int>::d; };
+template <class T, class U> typename G<T>::d foo (U);
+#define A(n) class A##n {};
+#define B(n) A(n##0) A(n##1) A(n##2) A(n##3) A(n##4) A(n##5) A(n##6) A(n##7) A(n##8) A(n##9)
+#define C(n) B(n##0) B(n##1) B(n##2) B(n##3) B(n##4) B(n##5) B(n##6) B(n##7) B(n##8) B(n##9)
+#define D(n) C(n##0) C(n##1) C(n##2) C(n##3) C(n##4)
+D(1)
+class H;
+template <typename>
+struct I
+{
+  bool bar ();
+#undef A
+#define A(n) void f##n (A##n *);
+D(1)
+  void baz ();
+};
+A1000 v;
+template <typename T>
+bool I<T>::bar ()
+{
+#undef A
+#define A(n) A##n k##n = *foo<A##n> (v); f##n (&k##n);
+D(1)
+  foo<H> (v);
+  baz ();
+  return false;
+}
+struct J : I<int>
+{
+  void qux () { bar (); }
+};