diff mbox

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

Message ID alpine.DEB.2.20.9.1602050751230.1231@idea
State New
Headers show

Commit Message

Patrick Palka Feb. 5, 2016, 12:54 p.m. UTC 
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

Comments

Jason Merrill Feb. 5, 2016, 2:13 p.m. UTC | #1 
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
Jason Merrill Feb. 18, 2016, 3:51 a.m. UTC | #2 
OK.

Jason
Patrick Palka Feb. 18, 2016, 6:25 p.m. UTC | #3 
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?
Jason Merrill Feb. 19, 2016, 3:41 p.m. UTC | #4 
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
Patrick Palka Feb. 19, 2016, 4:56 p.m. UTC | #5 
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.
Jason Merrill Feb. 19, 2016, 7:06 p.m. UTC | #6 
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
Patrick Palka Feb. 19, 2016, 9:14 p.m. UTC | #7 
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
>
diff mbox

Patch

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 ();
+}