[PATCH] c++/78771 ICE with inheriting ctor

[PATCH] c++/78771 ICE with inheriting ctor

[Nathan Sidwell]

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

Jason,
this patch fixes 78771, were an assert fires due to recursive 
instantiation of an inheriting ctor.  Normally when a recursive 
instantiation is needed, we've already constructed and registered the 
declaration, so simply return it.  For ctors though we need to construct 
the clones after we've instantiated the the master pattern (later in 
instantiate_template_1).  Hence any recursive instantiation of a cloned 
fn will barf, as we do.

Now, with an inherited ctor we have to deduce its exception spec and 
deletedness (deduce_inheriting_ctor).  That's fine, until one gets the 
perverse testcase here.  In figuring out what Middle ctor is needed by 
Middle(0), we end up trying to instantiate Derived::Derived (int) to see 
if Middle::Middle (Derived) is a viable candidate.  And that's the 
recursion, as Derived::Derived inherits from Middle::Middle.

Fixed by checking if the cloned instantiations actually exist before 
looking for them.  I think the only case this can occur is when SPEC is 
an inherited ctor, hence the assert.  Also, I don't think this can get 
us the wrong exept spec and deletedness -- it will be other (member) 
ctors that could change it, and we reconstruct the clones later anyway 
in the usual path.

(I tried creating the clones earlier, immediately after construction and 
registering the main function, but that didn't work)

ok?

nathan
-- 
Nathan Sidwell

[-- Attachment #2: ctor-78771.diff --]
[-- Type: text/x-patch, Size: 1744 bytes --]

2016-12-19  Nathan Sidwell  <nathan@acm.org>

	PR c++/78771
	* pt.c (instantiate_template_1): Check for recursive instantiation
	of inheriting constructor.

	PR c++/78771
	* g++.dg/cpp0x/pr78771.C: New.

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 243746)
+++ cp/pt.c	(working copy)
@@ -17717,10 +17717,22 @@ instantiate_template_1 (tree tmpl, tree
       if (spec == error_mark_node)
 	return error_mark_node;
 
+      /* If this is an inherited ctor, we can recursively clone it
+	 when deducing the validity of the ctor.  But we won't have
+	 cloned the function yet, so do it now.  We'll redo this
+	 later, but any recursive information learnt here can't
+	 change the validity.  */
+      if (!TREE_CHAIN (spec))
+	{
+	  gcc_assert (DECL_INHERITED_CTOR (spec));
+	  clone_function_decl (spec, /*update_method_vec_p=*/0);
+	}
+
       /* Look for the clone.  */
       FOR_EACH_CLONE (clone, spec)
 	if (DECL_NAME (clone) == DECL_NAME (tmpl))
 	  return clone;
+
       /* We should always have found the clone by now.  */
       gcc_unreachable ();
       return NULL_TREE;
Index: testsuite/g++.dg/cpp0x/pr78771.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr78771.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr78771.C	(working copy)
@@ -0,0 +1,27 @@
+// PR c++/78771
+// { dg-do compile { target c++11 } }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}

Re: [PATCH] c++/78771 ICE with inheriting ctor

[Jason Merrill]

On 12/19/2016 08:09 AM, Nathan Sidwell wrote:
> this patch fixes 78771, were an assert fires due to recursive
> instantiation of an inheriting ctor.  Normally when a recursive
> instantiation is needed, we've already constructed and registered the
> declaration, so simply return it.  For ctors though we need to construct
> the clones after we've instantiated the the master pattern (later in
> instantiate_template_1).  Hence any recursive instantiation of a cloned
> fn will barf, as we do.
>
> Now, with an inherited ctor we have to deduce its exception spec and
> deletedness (deduce_inheriting_ctor).  That's fine, until one gets the
> perverse testcase here.  In figuring out what Middle ctor is needed by
> Middle(0), we end up trying to instantiate Derived::Derived (int) to see
> if Middle::Middle (Derived) is a viable candidate.

Hmm, that seems like where the problem is.  We shouldn't try to 
instantiate the inheriting constructor until we've already chosen the 
base constructor; in the new model the inheriting constructor is just an 
implementation detail.

Jason

Re: [PATCH] c++/78771 ICE with inheriting ctor

[Nathan Sidwell]

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

On 01/04/2017 12:53 AM, Jason Merrill wrote:

> Hmm, that seems like where the problem is.  We shouldn't try to
> instantiate the inheriting constructor until we've already chosen the
> base constructor; in the new model the inheriting constructor is just an
> implementation detail.

Oh what fun.  This testcase behaves differently for C++17, C++11 
-fnew-inheriting-ctors and C++11 -fno-new-inheriting-ctors compilation 
modes.

Firstly, unpatched G++ is fine in C++17 mode, because:
   /* In C++17, "If the initializer expression is a prvalue and the
      cv-unqualified version of the source type is the same class as the 
class
      of the destination, the initializer expression is used to 
initialize the
      destination object."  Handle that here to avoid doing overload
      resolution.  */
and inside that we have:

       /* FIXME P0135 doesn't say how to handle direct initialization from a
	 type with a suitable conversion operator.  Let's handle it like
	 copy-initialization, but allowing explict conversions.  */

That conversion lookup short-circuits the subsequent overload resolution 
that would otherwise explode.

Otherwise, with -fnew-inheriting-ctors, you are indeed correct.  There 
needs to be a call to strip_inheriting_ctors in deduce_inheriting_ctor.

With -fno-new-inheriting-ctors we need the original patch I posted 
(included herein).  I suppose we might be able to remove the assert from 
strip_inheriting_ctors and always call that from deduce_inheriting_ctor, 
but that seems a bad idea to me.

I was unable to produce a c++17 testcase that triggered this problem by 
avoiding the above-mentioned overload resolution short-circuiting.

As -fnew-inheriting-ctors is a mangling-affecting flag, I guess we're 
stuck with it for the foreseable future.

ok?

nathan

-- 
Nathan Sidwell

[-- Attachment #2: ctor-78771-2.diff --]
[-- Type: text/x-patch, Size: 4218 bytes --]

2017-01-11  Nathan Sidwell  <nathan@acm.org>

	PR c++/78771
	* pt.c (instantiate_template_1): Check for recursive instantiation
	of inheriting constructor when not new-inheriting-ctor.
	* method.c (deduce_inheriting_ctor): Use originating ctor when
	new-inheriting-ctor.

	PR c++/78771
	* g++.dg/cpp0x/pr78771-old.C: New.
	* g++.dg/cpp0x/pr78771-new.C: New.
	* g++.dg/cpp1z/pr78771.C: New.

Index: cp/method.c
===================================================================
--- cp/method.c	(revision 244314)
+++ cp/method.c	(working copy)
@@ -1858,11 +1858,15 @@ deduce_inheriting_ctor (tree decl)
   gcc_assert (DECL_INHERITED_CTOR (decl));
   tree spec;
   bool trivial, constexpr_, deleted;
+
+  tree inherited = DECL_INHERITED_CTOR (decl);
+  if (flag_new_inheriting_ctors)
+    inherited = strip_inheriting_ctors (inherited);
+
   synthesized_method_walk (DECL_CONTEXT (decl), sfk_inheriting_constructor,
 			   false, &spec, &trivial, &deleted, &constexpr_,
-			   /*diag*/false,
-			   DECL_INHERITED_CTOR (decl),
-			   FUNCTION_FIRST_USER_PARMTYPE (decl));
+			   /*diag=*/false,
+			   inherited, FUNCTION_FIRST_USER_PARMTYPE (decl));
   if (TREE_CODE (inherited_ctor_binfo (decl)) != TREE_BINFO)
     /* Inherited the same constructor from different base subobjects.  */
     deleted = true;
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 244314)
+++ cp/pt.c	(working copy)
@@ -17963,10 +17963,22 @@ instantiate_template_1 (tree tmpl, tree
       if (spec == error_mark_node)
 	return error_mark_node;
 
+      /* If this is an inherited ctor, we can recursively clone it
+	 when deducing the validity of the ctor.  But we won't have
+	 cloned the function yet, so do it now.  We'll redo this
+	 later, but any recursive information learnt here can't
+	 change the validity.  */
+      if (!TREE_CHAIN (spec))
+	{
+	  gcc_assert (!flag_new_inheriting_ctors
+		      && DECL_INHERITED_CTOR (spec));
+	  clone_function_decl (spec, /*update_method_vec_p=*/0);
+	}
       /* Look for the clone.  */
       FOR_EACH_CLONE (clone, spec)
 	if (DECL_NAME (clone) == DECL_NAME (tmpl))
 	  return clone;
+
       /* We should always have found the clone by now.  */
       gcc_unreachable ();
       return NULL_TREE;
Index: testsuite/g++.dg/cpp0x/pr78771-new.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr78771-new.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr78771-new.C	(working copy)
@@ -0,0 +1,28 @@
+// PR c++/78771
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fnew-inheriting-ctors" }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}
Index: testsuite/g++.dg/cpp0x/pr78771-old.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr78771-old.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr78771-old.C	(working copy)
@@ -0,0 +1,28 @@
+// PR c++/78771
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fno-new-inheriting-ctors" }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}
Index: testsuite/g++.dg/cpp1z/pr78771.C
===================================================================
--- testsuite/g++.dg/cpp1z/pr78771.C	(revision 0)
+++ testsuite/g++.dg/cpp1z/pr78771.C	(working copy)
@@ -0,0 +1,27 @@
+// PR c++/78771
+// { dg-options -std=c++1z }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}

Re: [PATCH] c++/78771 ICE with inheriting ctor

[Nathan Sidwell]

ping?

On 01/11/2017 10:53 AM, Nathan Sidwell wrote:
> On 01/04/2017 12:53 AM, Jason Merrill wrote:
>
>> Hmm, that seems like where the problem is.  We shouldn't try to
>> instantiate the inheriting constructor until we've already chosen the
>> base constructor; in the new model the inheriting constructor is just an
>> implementation detail.
>
> Oh what fun.  This testcase behaves differently for C++17, C++11
> -fnew-inheriting-ctors and C++11 -fno-new-inheriting-ctors compilation
> modes.
>
> Firstly, unpatched G++ is fine in C++17 mode, because:
>   /* In C++17, "If the initializer expression is a prvalue and the
>      cv-unqualified version of the source type is the same class as the
> class
>      of the destination, the initializer expression is used to
> initialize the
>      destination object."  Handle that here to avoid doing overload
>      resolution.  */
> and inside that we have:
>
>       /* FIXME P0135 doesn't say how to handle direct initialization from a
>      type with a suitable conversion operator.  Let's handle it like
>      copy-initialization, but allowing explict conversions.  */
>
> That conversion lookup short-circuits the subsequent overload resolution
> that would otherwise explode.
>
> Otherwise, with -fnew-inheriting-ctors, you are indeed correct.  There
> needs to be a call to strip_inheriting_ctors in deduce_inheriting_ctor.
>
> With -fno-new-inheriting-ctors we need the original patch I posted
> (included herein).  I suppose we might be able to remove the assert from
> strip_inheriting_ctors and always call that from deduce_inheriting_ctor,
> but that seems a bad idea to me.
>
> I was unable to produce a c++17 testcase that triggered this problem by
> avoiding the above-mentioned overload resolution short-circuiting.
>
> As -fnew-inheriting-ctors is a mangling-affecting flag, I guess we're
> stuck with it for the foreseable future.
>
> ok?
>
> nathan
>


-- 
Nathan Sidwell

Re: [PATCH] c++/78771 ICE with inheriting ctor

[Jason Merrill]

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

On Wed, Jan 11, 2017 at 10:53 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 01/04/2017 12:53 AM, Jason Merrill wrote:
>
>> Hmm, that seems like where the problem is.  We shouldn't try to
>> instantiate the inheriting constructor until we've already chosen the
>> base constructor; in the new model the inheriting constructor is just an
>> implementation detail.
>
> Oh what fun.  This testcase behaves differently for C++17, C++11
> -fnew-inheriting-ctors and C++11 -fno-new-inheriting-ctors compilation
> modes.
>
> Firstly, unpatched G++ is fine in C++17 mode, because:
>   /* In C++17, "If the initializer expression is a prvalue and the
>      cv-unqualified version of the source type is the same class as the
> class
>      of the destination, the initializer expression is used to initialize
> the
>      destination object."  Handle that here to avoid doing overload
>      resolution.  */
> and inside that we have:
>
>       /* FIXME P0135 doesn't say how to handle direct initialization from a
>          type with a suitable conversion operator.  Let's handle it like
>          copy-initialization, but allowing explict conversions.  */
>
> That conversion lookup short-circuits the subsequent overload resolution
> that would otherwise explode.
>
> Otherwise, with -fnew-inheriting-ctors, you are indeed correct.  There needs
> to be a call to strip_inheriting_ctors in deduce_inheriting_ctor.

That doesn't seem quite right; in deducing the inheriting ctor we are
interested in what it actually calls, so we don't want to strip.  I
was thinking about changing when we do that deduction: we shouldn't be
calling deduce_inheriting_ctor until we actually know we're calling
this inheriting ctor.  I was thinking that would mean removing the
code in fn_type_unification with the comment

  /* After doing deduction with the inherited constructor, actually
return an
     instantiation of the inheriting constructor.  */

and then looking up the inheriting constructor somehow in
build_over_call.  But that gets to be a big change.

Something smaller would be moving the call to deduce_inheriting_ctor
to build_over_call; we can get away with that because calling is the
only way to refer to a constructor. What do you think of this
approach?

[-- Attachment #2: deduce-later.diff --]
[-- Type: text/plain, Size: 5029 bytes --]

commit 56586a488a27f2d5b502bd35aaec7225d0fb1d31
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Jan 25 16:52:28 2017 -0500

    deduce-late

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index a78e1a9..99c51f3 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7581,6 +7581,11 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
 	joust (cand, w->loser, 1, complain);
     }
 
+  /* OK, we're actually calling this inherited constructor; set its deletedness
+     appropriately.  */
+  if (DECL_INHERITED_CTOR (fn))
+    deduce_inheriting_ctor (fn);
+
   /* Make =delete work with SFINAE.  */
   if (DECL_DELETED_FN (fn) && !(complain & tf_error))
     return error_mark_node;
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index b7c26a1..03a9730 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -1197,8 +1197,6 @@ add_method (tree type, tree method, tree using_decl)
 			  SET_DECL_INHERITED_CTOR
 			    (fn, ovl_cons (DECL_INHERITED_CTOR (method),
 					   DECL_INHERITED_CTOR (fn)));
-			  /* Adjust deletedness and such.  */
-			  deduce_inheriting_ctor (fn);
 			  /* And discard the new one.  */
 			  return false;
 			}
diff --git a/gcc/cp/method.c b/gcc/cp/method.c
index 5b366f0..e80b806 100644
--- a/gcc/cp/method.c
+++ b/gcc/cp/method.c
@@ -1855,6 +1855,7 @@ explain_implicit_non_constexpr (tree decl)
 void
 deduce_inheriting_ctor (tree decl)
 {
+  decl = DECL_ORIGIN (decl);
   gcc_assert (DECL_INHERITED_CTOR (decl));
   tree spec;
   bool trivial, constexpr_, deleted;
@@ -1868,6 +1869,13 @@ deduce_inheriting_ctor (tree decl)
     deleted = true;
   DECL_DELETED_FN (decl) = deleted;
   TREE_TYPE (decl) = build_exception_variant (TREE_TYPE (decl), spec);
+
+  tree clone;
+  FOR_EACH_CLONE (clone, decl)
+    {
+      DECL_DELETED_FN (clone) = deleted;
+      TREE_TYPE (clone) = build_exception_variant (TREE_TYPE (clone), spec);
+    }
 }
 
 /* Implicitly declare the special function indicated by KIND, as a
@@ -1968,10 +1976,10 @@ implicitly_declare_fn (special_function_kind kind, tree type,
 
   bool trivial_p = false;
 
-  if (inherited_ctor && TREE_CODE (inherited_ctor) == TEMPLATE_DECL)
+  if (inherited_ctor)
     {
-      /* For an inheriting constructor template, just copy these flags from
-	 the inherited constructor template for now.  */
+      /* For an inheriting constructor, just copy these flags from the
+	 inherited constructor until deduce_inheriting_ctor.  */
       raises = TYPE_RAISES_EXCEPTIONS (TREE_TYPE (inherited_ctor));
       deleted_p = DECL_DELETED_FN (inherited_ctor);
       constexpr_p = DECL_DECLARED_CONSTEXPR_P (inherited_ctor);
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 537d107..c96ceed 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12358,8 +12358,6 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	    maybe_retrofit_in_chrg (r);
 	    if (DECL_CONSTRUCTOR_P (r))
 	      grok_ctor_properties (ctx, r);
-	    if (DECL_INHERITED_CTOR (r))
-	      deduce_inheriting_ctor (r);
 	    /* If this is an instantiation of a member template, clone it.
 	       If it isn't, that'll be handled by
 	       clone_constructors_and_destructors.  */
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr78771-new.C b/gcc/testsuite/g++.dg/cpp0x/pr78771-new.C
new file mode 100644
index 0000000..f489f86
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr78771-new.C
@@ -0,0 +1,28 @@
+// PR c++/78771
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fnew-inheriting-ctors" }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr78771-old.C b/gcc/testsuite/g++.dg/cpp0x/pr78771-old.C
new file mode 100644
index 0000000..b723b11
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr78771-old.C
@@ -0,0 +1,28 @@
+// PR c++/78771
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fno-new-inheriting-ctors" }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}
diff --git a/gcc/testsuite/g++.dg/cpp1z/pr78771.C b/gcc/testsuite/g++.dg/cpp1z/pr78771.C
new file mode 100644
index 0000000..9178494
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/pr78771.C
@@ -0,0 +1,27 @@
+// PR c++/78771
+// { dg-options -std=c++1z }
+
+// ICE instantiating a deleted inherited ctor
+
+struct Base
+{
+  template <typename U> Base (U);
+
+  Base (int);
+};
+
+struct Derived;
+
+struct Middle : Base
+{
+  using Base::Base;
+
+  Middle (Derived);
+};
+
+struct Derived : Middle
+{
+  using Middle::Middle;
+};
+
+Middle::Middle (Derived) : Middle (0) {}

Re: [PATCH] c++/78771 ICE with inheriting ctor

[Nathan Sidwell]

On 01/25/2017 05:09 PM, Jason Merrill wrote:

> Something smaller would be moving the call to deduce_inheriting_ctor
> to build_over_call; we can get away with that because calling is the
> only way to refer to a constructor. What do you think of this
> approach?

LGTM, thanks!

nathan

-- 
Nathan Sidwell