[PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

The compiler correctly detects and diagnoses invalid constructor calls
such as C::C () in a non-template context but it fails to do so while
processing a class template.  [ Section 3.4.3.1 of the standard is what
makes these forms of constructor calls illegal -- see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68948#c9  ]

In a non-template context this diagnosis would take place in
build_new_method_call, called from finish_call_expr, but while
processing a class template we may exit early out of finish_call_expr
and never call build_new_method_call.

Thus we never diagnose this invalid constructor call during template
processing.  So then during instantiation of the enclosing template we
call tsubst_baselink on this constructor call, during which the call to
lookup_fnfields returns NULL (because it finds the injected class type C
not the constructor C).  Because the lookup failed, tsubst_baselink
returns error_mark_node and this node persists all the way through to
gimplification where it silently gets discarded.

This patch fixes this problem by diagnosing these invalid constructor
calls in tsubst_baselink.  Alternatively, we can rewire finish_call_expr
avoid exiting early while processing a template if the call in question
is a constructor call.  I'm not sure which approach is better.  This
approach seems more conservative, since it's just attaching an error
message to an existing error path.

gcc/cp/ChangeLog:

	PR c++/68948
	* pt.c (tsubst_baselink): Diagnose an invalid constructor call
	if lookup_fnfields returns NULL_TREE and name we're looking up
	has the form A::A.

gcc/testsuite/ChangeLog:

	PR c++/68948
	* g++.dg/template/pr68948.C: New test.
---
 gcc/cp/pt.c                             | 10 +++++++-
 gcc/testsuite/g++.dg/template/pr68948.C | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/template/pr68948.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 4d405cf..91d0ef2 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -13583,7 +13583,15 @@ tsubst_baselink (tree baselink, tree object_type,
       name = mangle_conv_op_name_for_type (optype);
     baselink = lookup_fnfields (qualifying_scope, name, /*protect=*/1);
     if (!baselink)
-      return error_mark_node;
+      {
+	if (constructor_name_p (name, qualifying_scope))
+	  {
+	    if (complain & tf_error)
+	      error ("cannot call constructor %<%T::%D%> directly",
+		     qualifying_scope, name);
+	  }
+	return error_mark_node;
+      }
 
     /* If lookup found a single function, mark it as used at this
        point.  (If it lookup found multiple functions the one selected
diff --git a/gcc/testsuite/g++.dg/template/pr68948.C b/gcc/testsuite/g++.dg/template/pr68948.C
new file mode 100644
index 0000000..ccbfb19
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr68948.C
@@ -0,0 +1,41 @@
+// PR c++/68948
+
+struct B { B (); B (int); };
+
+struct Time : B { };
+
+/* Here, A and B are unrelated types.  */
+
+template <typename>
+struct A
+{
+  void TestBody ()
+  {
+    B::B (); // { dg-error "cannot call constructor" }
+    B::B::B (); // { dg-error "cannot call constructor" }
+    B::B (0); // { dg-error "cannot call constructor" }
+  }
+};
+
+/* Here, C is (indirectly) derived from B.  */
+
+template <typename g>
+struct C : Time
+{
+  void TestBody ()
+  {
+    B::B (); // { dg-error "cannot call constructor" }
+    B::B::B (); // { dg-error "cannot call constructor" }
+    B::B (0); // { dg-error "cannot call constructor" }
+    Time::B (0);
+  }
+};
+
+int
+main (void)
+{
+  A<int> a;
+  C<int> c;
+  a.TestBody ();
+  c.TestBody ();
+}
-- 
2.7.0.303.g36d4cae

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

On Thu, 4 Feb 2016, Patrick Palka wrote:

> The compiler correctly detects and diagnoses invalid constructor calls
> such as C::C () in a non-template context but it fails to do so while
> processing a class template.  [ Section 3.4.3.1 of the standard is what
> makes these forms of constructor calls illegal -- see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68948#c9  ]
>
> In a non-template context this diagnosis would take place in
> build_new_method_call, called from finish_call_expr, but while
> processing a class template we may exit early out of finish_call_expr
> and never call build_new_method_call.
>
> Thus we never diagnose this invalid constructor call during template
> processing.  So then during instantiation of the enclosing template we
> call tsubst_baselink on this constructor call, during which the call to
> lookup_fnfields returns NULL (because it finds the injected class type C
> not the constructor C).  Because the lookup failed, tsubst_baselink
> returns error_mark_node and this node persists all the way through to
> gimplification where it silently gets discarded.
>
> This patch fixes this problem by diagnosing these invalid constructor
> calls in tsubst_baselink.  Alternatively, we can rewire finish_call_expr
> avoid exiting early while processing a template if the call in question
> is a constructor call.  I'm not sure which approach is better.  This
> approach seems more conservative, since it's just attaching an error
> message to an existing error path.

And here is the other approach, which rewires finish_call_expr:

-- >8 --

gcc/cp/ChangeLog:

 	PR c++/68948
 	* semantics.c (finish_call_expr): Don't assume a constructor
 	call is dependent if the "this" pointer is dependent.

gcc/testsuite/ChangeLog:

 	PR c++/68948
 	* g++.dg/template/pr68948.C: New test.
---
  gcc/cp/semantics.c                      | 14 +++++++++--
  gcc/testsuite/g++.dg/template/pr68948.C | 41 +++++++++++++++++++++++++++++++++
  2 files changed, 53 insertions(+), 2 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/template/pr68948.C

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 95c4f19..31c03ae 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -2270,6 +2270,7 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
  	     related to CWG issues 515 and 1005.  */
  	  || (TREE_CODE (fn) != COMPONENT_REF
  	      && non_static_member_function_p (fn)
+	      && !DECL_MAYBE_IN_CHARGE_CONSTRUCTOR_P (get_first_fn (fn))
  	      && current_class_ref
  	      && type_dependent_expression_p (current_class_ref)))
  	{
@@ -2348,8 +2349,17 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
  	[class.access.base] says that we need to convert 'this' to B* as
  	part of the access, so we pass 'B' to maybe_dummy_object.  */

-      object = maybe_dummy_object (BINFO_TYPE (BASELINK_ACCESS_BINFO (fn)),
-				   NULL);
+      if (DECL_MAYBE_IN_CHARGE_CONSTRUCTOR_P (get_first_fn (fn)))
+	{
+	  /* A constructor call always uses a dummy object.  (This constructor
+	     call which has the form A::A () is actually invalid and we are
+	     going to reject it later in build_new_method_call.)  */
+	  object = build_dummy_object (BINFO_TYPE (BASELINK_ACCESS_BINFO (fn)));
+	  gcc_assert (!type_dependent_expression_p (object));
+	}
+      else
+	object = maybe_dummy_object (BINFO_TYPE (BASELINK_ACCESS_BINFO (fn)),
+				     NULL);

        if (processing_template_decl)
  	{
diff --git a/gcc/testsuite/g++.dg/template/pr68948.C b/gcc/testsuite/g++.dg/template/pr68948.C
new file mode 100644
index 0000000..3cb6844
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr68948.C
@@ -0,0 +1,41 @@
+// PR c++/68948
+
+struct B { B (); B (int); };
+
+struct Time : B { };
+
+/* Here, A and B are unrelated types.  */
+
+template <typename>
+struct A
+{
+  void TestBody ()
+  {
+    B::B (); // { dg-error "cannot call constructor .B::B." }
+    B::B::B (); // { dg-error "cannot call constructor .B::B." }
+    B::B (0); // { dg-error "cannot call constructor .B::B." }
+  }
+};
+
+/* Here, C is (indirectly) derived from B.  */
+
+template <typename g>
+struct C : Time
+{
+  void TestBody ()
+  {
+    B::B (); // { dg-error "cannot call constructor .B::B." }
+    B::B::B (); // { dg-error "cannot call constructor .B::B." }
+    B::B (0); // { dg-error "cannot call constructor .B::B." }
+    Time::B (0);
+  }
+};
+
+int
+main (void)
+{
+  A<int> a;
+  C<int> c;
+  a.TestBody ();
+  c.TestBody ();
+}
-- 
2.7.0.303.g36d4cae

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Jason Merrill]

On 02/05/2016 07:54 AM, Patrick Palka wrote:
> On Thu, 4 Feb 2016, Patrick Palka wrote:
>
>> The compiler correctly detects and diagnoses invalid constructor calls
>> such as C::C () in a non-template context but it fails to do so while
>> processing a class template.  [ Section 3.4.3.1 of the standard is what
>> makes these forms of constructor calls illegal -- see
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68948#c9  ]
>>
>> In a non-template context this diagnosis would take place in
>> build_new_method_call, called from finish_call_expr, but while
>> processing a class template we may exit early out of finish_call_expr
>> and never call build_new_method_call.
>>
>> Thus we never diagnose this invalid constructor call during template
>> processing.  So then during instantiation of the enclosing template we
>> call tsubst_baselink on this constructor call, during which the call to
>> lookup_fnfields returns NULL (because it finds the injected class type C
>> not the constructor C).  Because the lookup failed, tsubst_baselink
>> returns error_mark_node and this node persists all the way through to
>> gimplification where it silently gets discarded.
>>
>> This patch fixes this problem by diagnosing these invalid constructor
>> calls in tsubst_baselink.  Alternatively, we can rewire finish_call_expr
>> avoid exiting early while processing a template if the call in question
>> is a constructor call.  I'm not sure which approach is better.  This
>> approach seems more conservative, since it's just attaching an error
>> message to an existing error path.
>
> And here is the other approach, which rewires finish_call_expr:

I like the second approach better, but you're right that the first is 
more conservative, so let's go with the first for GCC 6 and switch to 
the second for GCC 7.

Jason

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Jason Merrill]

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

On 02/05/2016 09:13 AM, Jason Merrill wrote:
> On 02/05/2016 07:54 AM, Patrick Palka wrote:
>> On Thu, 4 Feb 2016, Patrick Palka wrote:
>>
>>> The compiler correctly detects and diagnoses invalid constructor calls
>>> such as C::C () in a non-template context but it fails to do so while
>>> processing a class template.  [ Section 3.4.3.1 of the standard is what
>>> makes these forms of constructor calls illegal -- see
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68948#c9  ]
>>>
>>> In a non-template context this diagnosis would take place in
>>> build_new_method_call, called from finish_call_expr, but while
>>> processing a class template we may exit early out of finish_call_expr
>>> and never call build_new_method_call.
>>>
>>> Thus we never diagnose this invalid constructor call during template
>>> processing.  So then during instantiation of the enclosing template we
>>> call tsubst_baselink on this constructor call, during which the call to
>>> lookup_fnfields returns NULL (because it finds the injected class type C
>>> not the constructor C).  Because the lookup failed, tsubst_baselink
>>> returns error_mark_node and this node persists all the way through to
>>> gimplification where it silently gets discarded.
>>>
>>> This patch fixes this problem by diagnosing these invalid constructor
>>> calls in tsubst_baselink.  Alternatively, we can rewire finish_call_expr
>>> avoid exiting early while processing a template if the call in question
>>> is a constructor call.  I'm not sure which approach is better.  This
>>> approach seems more conservative, since it's just attaching an error
>>> message to an existing error path.
>>
>> And here is the other approach, which rewires finish_call_expr:
>
> I like the second approach better, but you're right that the first is
> more conservative, so let's go with the first for GCC 6 and switch to
> the second for GCC 7.

I'm also applying this patch so that similar issues ICE rather than 
silently generate bad code.


[-- Attachment #2: 68948-ice.patch --]
[-- Type: text/x-patch, Size: 707 bytes --]

commit f15c5ca5e31d39fb13ef700afeb43aad0c8c7903
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Feb 5 10:34:33 2016 -0500

    	Make issues similar to PR c++/68948 fail loudly.
    
    	* semantics.c (finish_expr_stmt): If expr is error_mark_node,
    	make sure we've seen_error().

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 95c4f19..c9f9db4 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -673,6 +673,9 @@ finish_expr_stmt (tree expr)
 
   if (expr != NULL_TREE)
     {
+      /* If we ran into a problem, make sure we complained.  */
+      gcc_assert (expr != error_mark_node || seen_error ());
+
       if (!processing_template_decl)
 	{
 	  if (warn_sequence_point)

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

On Fri, Feb 5, 2016 at 12:51 PM, Jason Merrill <jason@redhat.com> wrote:
> On 02/05/2016 09:13 AM, Jason Merrill wrote:
>>
>> On 02/05/2016 07:54 AM, Patrick Palka wrote:
>>>
>>> On Thu, 4 Feb 2016, Patrick Palka wrote:
>>>
>>>> The compiler correctly detects and diagnoses invalid constructor calls
>>>> such as C::C () in a non-template context but it fails to do so while
>>>> processing a class template.  [ Section 3.4.3.1 of the standard is what
>>>> makes these forms of constructor calls illegal -- see
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68948#c9  ]
>>>>
>>>> In a non-template context this diagnosis would take place in
>>>> build_new_method_call, called from finish_call_expr, but while
>>>> processing a class template we may exit early out of finish_call_expr
>>>> and never call build_new_method_call.
>>>>
>>>> Thus we never diagnose this invalid constructor call during template
>>>> processing.  So then during instantiation of the enclosing template we
>>>> call tsubst_baselink on this constructor call, during which the call to
>>>> lookup_fnfields returns NULL (because it finds the injected class type C
>>>> not the constructor C).  Because the lookup failed, tsubst_baselink
>>>> returns error_mark_node and this node persists all the way through to
>>>> gimplification where it silently gets discarded.
>>>>
>>>> This patch fixes this problem by diagnosing these invalid constructor
>>>> calls in tsubst_baselink.  Alternatively, we can rewire finish_call_expr
>>>> avoid exiting early while processing a template if the call in question
>>>> is a constructor call.  I'm not sure which approach is better.  This
>>>> approach seems more conservative, since it's just attaching an error
>>>> message to an existing error path.
>>>
>>>
>>> And here is the other approach, which rewires finish_call_expr:
>>
>>
>> I like the second approach better, but you're right that the first is
>> more conservative, so let's go with the first for GCC 6 and switch to
>> the second for GCC 7.
>
>
> I'm also applying this patch so that similar issues ICE rather than silently
> generate bad code.
>

Cool! Good idea.

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Jason Merrill]

OK.

Jason

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

On Wed, Feb 17, 2016 at 10:51 PM, Jason Merrill <jason@redhat.com> wrote:
> OK.

Is this an approval of the 2nd patch for next stage 1?

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Jason Merrill]

On 02/18/2016 01:25 PM, Patrick Palka wrote:
> On Wed, Feb 17, 2016 at 10:51 PM, Jason Merrill <jason@redhat.com> wrote:
>> OK.
>
> Is this an approval of the 2nd patch for next stage 1?

Actually, I've been looking at this area a lot recently in the context 
of the 10200 fix, and now I think we can go ahead with the 2nd patch 
now, but without the assert; I think it would fire if we wrote A::A().

Jason

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

On Fri, Feb 19, 2016 at 10:41 AM, Jason Merrill <jason@redhat.com> wrote:
> On 02/18/2016 01:25 PM, Patrick Palka wrote:
>>
>> On Wed, Feb 17, 2016 at 10:51 PM, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> OK.
>>
>>
>> Is this an approval of the 2nd patch for next stage 1?
>
>
> Actually, I've been looking at this area a lot recently in the context of
> the 10200 fix, and now I think we can go ahead with the 2nd patch now, but
> without the assert; I think it would fire if we wrote A::A().

I w ill commit the version without the assert shortly, but...

I haven't been able to get the assert to fire even when the A in
A::A() is dependent because in that case FN should be dependent, so we
would already have exited out of finish_call_expr due to the
type_dependent_expression_p (fn) check near the top of
finish_call_expr.  (In particular for dependent A::A(), FN is a
SCOPE_REF whose 1st operand is the dependent type A and 2nd operand is
the identifier node A.)

So it seems to me that the assert at that location is safe, since the
dummy object should be dependent only if the constructor call FN is
dependent in which case we would never reach the assert.

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Jason Merrill]

On 02/19/2016 11:56 AM, Patrick Palka wrote:
> On Fri, Feb 19, 2016 at 10:41 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 02/18/2016 01:25 PM, Patrick Palka wrote:
>>>
>>> On Wed, Feb 17, 2016 at 10:51 PM, Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> OK.
>>>
>>> Is this an approval of the 2nd patch for next stage 1?
>>
>> Actually, I've been looking at this area a lot recently in the context of
>> the 10200 fix, and now I think we can go ahead with the 2nd patch now, but
>> without the assert; I think it would fire if we wrote A::A().
>
> I w ill commit the version without the assert shortly, but...
>
> I haven't been able to get the assert to fire even when the A in
> A::A() is dependent because in that case FN should be dependent, so we
> would already have exited out of finish_call_expr due to the
> type_dependent_expression_p (fn) check near the top of
> finish_call_expr.  (In particular for dependent A::A(), FN is a
> SCOPE_REF whose 1st operand is the dependent type A and 2nd operand is
> the identifier node A.)
>
> So it seems to me that the assert at that location is safe, since the
> dummy object should be dependent only if the constructor call FN is
> dependent in which case we would never reach the assert.

It might be safe given our current dependency analysis, but I'm planning 
to tighten that up in GCC 7, along the lines of my 69753 patch that I 
ended up backing out.  Why do you want the assert?

Jason

Re: [PATCH] Fix PR c++/68948 (wrong code generation due to invalid constructor call)

[Patrick Palka]

On Fri, Feb 19, 2016 at 2:06 PM, Jason Merrill <jason@redhat.com> wrote:
> On 02/19/2016 11:56 AM, Patrick Palka wrote:
>>
>> On Fri, Feb 19, 2016 at 10:41 AM, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> On 02/18/2016 01:25 PM, Patrick Palka wrote:
>>>>
>>>>
>>>> On Wed, Feb 17, 2016 at 10:51 PM, Jason Merrill <jason@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> OK.
>>>>
>>>>
>>>> Is this an approval of the 2nd patch for next stage 1?
>>>
>>>
>>> Actually, I've been looking at this area a lot recently in the context of
>>> the 10200 fix, and now I think we can go ahead with the 2nd patch now,
>>> but
>>> without the assert; I think it would fire if we wrote A::A().
>>
>>
>> I w ill commit the version without the assert shortly, but...
>>
>> I haven't been able to get the assert to fire even when the A in
>> A::A() is dependent because in that case FN should be dependent, so we
>> would already have exited out of finish_call_expr due to the
>> type_dependent_expression_p (fn) check near the top of
>> finish_call_expr.  (In particular for dependent A::A(), FN is a
>> SCOPE_REF whose 1st operand is the dependent type A and 2nd operand is
>> the identifier node A.)
>>
>> So it seems to me that the assert at that location is safe, since the
>> dummy object should be dependent only if the constructor call FN is
>> dependent in which case we would never reach the assert.
>
>
> It might be safe given our current dependency analysis, but I'm planning to
> tighten that up in GCC 7, along the lines of my 69753 patch that I ended up
> backing out.  Why do you want the assert?

No good reason.

>
> Jason
>