[C++ PATCH] 79393 fix

[C++ PATCH] 79393 fix

[Nathan Sidwell]

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

The resolution to DR 1658 causes vbases of abstract classes to be 
ignored when building the cdtors.  That made sense for ctors, when there 
might be no default ctor available.  But for dtors, there is only oe 
dtor and they can be virtual.  That can break virtual overriding, as 
79393 discovered.

I've reported this as a DR, but so GCC 7 doesn't break currently working 
code, I've committed this patch to skip that part of 1658's resolution.

The inhibiting of access checks was needed of pr66443-cxx14-3.C to 
continue passing.  That's a case where the vbase's dtor is inaccessible 
from the abstract base.  That seemed like the most conservative way to 
inhibit 1658's impact by allowing the most code to be compiled, but of 
course is speculative.

nathan
-- 
Nathan Sidwell

[-- Attachment #2: vbase-79393.diff --]
[-- Type: text/x-patch, Size: 2345 bytes --]

2017-03-13  Nathan Sidwell  <nathan@acm.org>

	PR c++/79393 DR 1658 workaround
	* method.c (synthesized_method_walk): Check vbases of abstract
	classes for dtor walk.

Index: cp/method.c
===================================================================
--- cp/method.c	(revision 246084)
+++ cp/method.c	(working copy)
@@ -1664,12 +1664,26 @@ synthesized_method_walk (tree ctype, spe
     /* Already examined vbases above.  */;
   else if (vec_safe_is_empty (vbases))
     /* No virtual bases to worry about.  */;
-  else if (ABSTRACT_CLASS_TYPE_P (ctype) && cxx_dialect >= cxx14)
+  else if (ABSTRACT_CLASS_TYPE_P (ctype) && cxx_dialect >= cxx14
+	   /* DR 1658 specifies that vbases of abstract classes are
+	      ignored for both ctors and dtors.  However, that breaks
+	      virtual dtor overriding when the ignored base has a
+	      throwing destructor.  So, ignore that piece of 1658.  A
+	      defect has been filed (no number yet).  */
+	   && sfk != sfk_destructor)
     /* Vbase cdtors are not relevant.  */;
   else
     {
       if (constexpr_p)
 	*constexpr_p = false;
+
+      /* To be conservative, ignore access to the base dtor that
+	 DR1658 instructs us to ignore.  */
+      bool no_access_check = (cxx_dialect >= cxx14
+			      && ABSTRACT_CLASS_TYPE_P (ctype));
+
+      if (no_access_check)
+	push_deferring_access_checks (dk_no_check);
       FOR_EACH_VEC_ELT (*vbases, i, base_binfo)
 	synthesized_method_base_walk (binfo, base_binfo, quals,
 				      copy_arg_p, move_p, ctor_p,
@@ -1677,6 +1691,8 @@ synthesized_method_walk (tree ctype, spe
 				      fnname, flags, diag,
 				      spec_p, trivial_p,
 				      deleted_p, constexpr_p);
+      if (no_access_check)
+	pop_deferring_access_checks ();
     }
 
   /* Now handle the non-static data members.  */
Index: testsuite/g++.dg/cpp1y/pr79393.C
===================================================================
--- testsuite/g++.dg/cpp1y/pr79393.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr79393.C	(working copy)
@@ -0,0 +1,21 @@
+// { dg-do compile { target c++14 } }
+// PR c++/79393 deduced eh spec, deleted dtors and vbases
+
+struct A3;
+
+struct VDT {
+  virtual ~VDT () noexcept (false);
+};
+
+struct A1 : virtual VDT {
+  virtual void abstract () = 0;
+};
+
+struct A2 : A1 {  };
+
+struct A3 : A2 
+{
+  virtual void abstract ();
+};
+
+A3 a3;

Re: [C++ PATCH] 79393 fix

[Jason Merrill]

On Mon, Mar 13, 2017 at 8:06 AM, Nathan Sidwell <nathan@acm.org> wrote:
> The resolution to DR 1658 causes vbases of abstract classes to be ignored
> when building the cdtors.  That made sense for ctors, when there might be no
> default ctor available.  But for dtors, there is only oe dtor and they can
> be virtual.  That can break virtual overriding, as 79393 discovered.
>
> I've reported this as a DR, but so GCC 7 doesn't break currently working
> code, I've committed this patch to skip that part of 1658's resolution.
>
> The inhibiting of access checks was needed of pr66443-cxx14-3.C to continue
> passing.  That's a case where the vbase's dtor is inaccessible from the
> abstract base.  That seemed like the most conservative way to inhibit 1658's
> impact by allowing the most code to be compiled, but of course is
> speculative.

It looks like you're ignoring the access for all base destructors;
handling this in synthesized_method_base_walk would let you limit the
change to vbases with virtual destructors.  That function also already
handles ignoring access control for an inherited constructor.

Jason

Re: [C++ PATCH] 79393 fix

[Nathan Sidwell]

On 03/13/2017 05:05 PM, Jason Merrill wrote:

> It looks like you're ignoring the access for all base destructors;
> handling this in synthesized_method_base_walk would let you limit the
> change to vbases with virtual destructors.  That function also already
> handles ignoring access control for an inherited constructor.

d'oh! you're right.  I was in two minds about ignoring access at all -- and 
xfailing that testcase for the moment.  That would avoid an unfortunate possible 
future where the DR resolves to revert to the pre DR 1658 behaviour for dtors. 
thoughts?

nathan

-- 
Nathan Sidwell

Re: [C++ PATCH] 79393 fix

[Nathan Sidwell]

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

On 03/13/2017 06:20 PM, Nathan Sidwell wrote:
> On 03/13/2017 05:05 PM, Jason Merrill wrote:
>
>> It looks like you're ignoring the access for all base destructors;
>> handling this in synthesized_method_base_walk would let you limit the
>> change to vbases with virtual destructors.  That function also already
>> handles ignoring access control for an inherited constructor.

Committed this.  It looked to me that the inheriting ctor case should be 
using dk_no_check rather than dk_deferring, given the comment.  But I 
didn't want to change that in this patch.

nathan

-- 
Nathan Sidwell

[-- Attachment #2: vdtor-79393-3.diff --]
[-- Type: text/x-patch, Size: 3808 bytes --]

2017-03-14  Nathan Sidwell  <nathan@acm.org>

	PR c++/79393 DR 1658 workaround
	* method.c (synthesized_method_base_walk): Inihibit abstract class
	virtual base access check here.
	(synthesized_method_walk): Not here.

Index: cp/method.c
===================================================================
--- cp/method.c	(revision 246120)
+++ cp/method.c	(working copy)
@@ -1420,10 +1420,10 @@ walk_field_subobs (tree fields, tree fnn
     }
 }
 
-// Base walker helper for synthesized_method_walk.  Inspect a direct
-// or virtual base.  BINFO is the parent type's binfo.  BASE_BINFO is
-// the base binfo of interests.  All other parms are as for
-// synthesized_method_walk, or its local vars.
+/* Base walker helper for synthesized_method_walk.  Inspect a direct
+   or virtual base.  BINFO is the parent type's binfo.  BASE_BINFO is
+   the base binfo of interests.  All other parms are as for
+   synthesized_method_walk, or its local vars.  */
 
 static tree
 synthesized_method_base_walk (tree binfo, tree base_binfo, 
@@ -1436,7 +1436,8 @@ synthesized_method_base_walk (tree binfo
 {
   bool inherited_binfo = false;
   tree argtype = NULL_TREE;
-  
+  deferring_kind defer = dk_no_deferred;
+
   if (copy_arg_p)
     argtype = build_stub_type (BINFO_TYPE (base_binfo), quals, move_p);
   else if ((inherited_binfo
@@ -1445,11 +1446,21 @@ synthesized_method_base_walk (tree binfo
       argtype = inherited_parms;
       /* Don't check access on the inherited constructor.  */
       if (flag_new_inheriting_ctors)
-	push_deferring_access_checks (dk_deferred);
+	defer = dk_deferred;
     }
+  /* To be conservative, ignore access to the base dtor that
+     DR1658 instructs us to ignore.  See the comment in
+     synthesized_method_walk.  */
+  else if (cxx_dialect >= cxx14 && fnname == complete_dtor_identifier
+	   && BINFO_VIRTUAL_P (base_binfo)
+	   && ABSTRACT_CLASS_TYPE_P (BINFO_TYPE (binfo)))
+    defer = dk_no_check;
+
+  if (defer != dk_no_deferred)
+    push_deferring_access_checks (defer);
   tree rval = locate_fn_flags (base_binfo, fnname, argtype, flags,
 			       diag ? tf_warning_or_error : tf_none);
-  if (inherited_binfo && flag_new_inheriting_ctors)
+  if (defer != dk_no_deferred)
     pop_deferring_access_checks ();
 
   process_subob_fn (rval, spec_p, trivial_p, deleted_p,
@@ -1677,13 +1688,6 @@ synthesized_method_walk (tree ctype, spe
       if (constexpr_p)
 	*constexpr_p = false;
 
-      /* To be conservative, ignore access to the base dtor that
-	 DR1658 instructs us to ignore.  */
-      bool no_access_check = (cxx_dialect >= cxx14
-			      && ABSTRACT_CLASS_TYPE_P (ctype));
-
-      if (no_access_check)
-	push_deferring_access_checks (dk_no_check);
       FOR_EACH_VEC_ELT (*vbases, i, base_binfo)
 	synthesized_method_base_walk (binfo, base_binfo, quals,
 				      copy_arg_p, move_p, ctor_p,
@@ -1691,8 +1695,6 @@ synthesized_method_walk (tree ctype, spe
 				      fnname, flags, diag,
 				      spec_p, trivial_p,
 				      deleted_p, constexpr_p);
-      if (no_access_check)
-	pop_deferring_access_checks ();
     }
 
   /* Now handle the non-static data members.  */
Index: testsuite/g++.dg/cpp1y/pr79393-2.C
===================================================================
--- testsuite/g++.dg/cpp1y/pr79393-2.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr79393-2.C	(working copy)
@@ -0,0 +1,22 @@
+// { dg-do compile { target c++14 } }
+
+// DR 1658, inaccessible dtor of virtual base doesn't affect an
+// abstract class.  But we should stil check access to non-virtual bases.
+
+class C;
+
+struct A {
+private:
+  ~A (){  }
+  friend class C;
+};
+
+struct B : A { // { dg-error "is private" }
+  virtual bool Ok () = 0; // abstract
+};
+
+struct C : B {
+  ~C () 
+  { }  // { dg-error "use of deleted" }
+  virtual bool Ok ();
+};

Re: [C++ PATCH] 79393 fix

[Jason Merrill]

On Tue, Mar 14, 2017 at 10:43 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 03/13/2017 06:20 PM, Nathan Sidwell wrote:
>>
>> On 03/13/2017 05:05 PM, Jason Merrill wrote:
>>
>>> It looks like you're ignoring the access for all base destructors;
>>> handling this in synthesized_method_base_walk would let you limit the
>>> change to vbases with virtual destructors.  That function also already
>>> handles ignoring access control for an inherited constructor.
>
>
> Committed this.  It looked to me that the inheriting ctor case should be
> using dk_no_check rather than dk_deferring, given the comment.  But I didn't
> want to change that in this patch.

The difference is that dk_no_check overrides any further pushes; see
push_deferring_access_checks.  I don't remember why that is, but
that's why we instead use dk_deferring and then throw away the
deferred checks.

Jason