diff mbox

[RFH,/] PR 54191

Message ID 50215C50.2080708@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Aug. 7, 2012, 6:20 p.m. UTC
Hi,

this is an update on C++/54191, where in a number of cases, having to do 
with inaccessible bases, we don't handle correctly access control under 
SFINAE.

The attached draft patch p fixes a number of tests (and passes 
regression testing), where we are currently emitting inaccessible base 
diagnostics from lookup_base instead of properly handling the failure in 
SFINAE. As you can see in the draft, after a number of tries, I'm simply 
adding a tsubst_flags_t parameter to lookup_base: obviously something is 
redundant with base_access, but knowing that a returned error_mark_node 
in every case means that the base is either ambiguous or inaccessible 
(when we error out outside SFINAE) is *so* convenient! For comparison, 
the existing get_delta_difference_1 shows an alternate approach, quite 
contorted IMHO. But these are just implementation details...

More interesting are the cases, commented out in the attached 54191.C, 
which we still get wrong also with the patch applied. In those cases we 
do *not* currently produce any inaccessible base diagnostics, we simply 
mishandle the SFINAE, apparently we don't perform any form of access 
control. I'm really looking for help about this set of 5 tests (4 braced 
casts + conditional operator)

Thanks!
Paolo.

///////////////////////////

Comments

Paolo Carlini Aug. 8, 2012, 11:57 a.m. UTC | #1
.. I'm coming to the conclusion that the tests which are not fixed by a 
patch along the lines of my draft don't have much to do with SFINAE vs 
inaccessible bases per se (with the possible exception of the 
conditional operator case). Consider:

struct A
{};

struct B
{};

template<typename T>
T &&declval();

template<typename From, typename = decltype(B{declval<From>()})>
constexpr bool test_braced_cast_to_base(int)
{ return true; }

template<typename>
constexpr bool test_braced_cast_to_base(bool)
{ return false; }

static_assert(!test_braced_cast_to_base<A>(0), "");

this also triggers the static_assert. Really, in 
'decltype(B{declval<From>()})' almost *everything* is Ok between the 
curly brackets. Maybe we should have a separate PR for this.

Paolo.
Paolo Carlini Aug. 8, 2012, 12:47 p.m. UTC | #2
On 08/08/2012 01:57 PM, Paolo Carlini wrote:
> this also triggers the static_assert. Really, in 
> 'decltype(B{declval<From>()})' almost *everything* is Ok between the 
> curly brackets. Maybe we should have a separate PR for this.
And I think this issue is addressed by the ongoing work on instantiation 
dependency, thus C++/51222 etc.

AFAICS, the most uncertain case is the conditional operator test, 
otherwise we could split the PR.

Paolo.
Paolo Carlini Aug. 8, 2012, 12:58 p.m. UTC | #3
On 08/08/2012 02:47 PM, Paolo Carlini wrote:
> On 08/08/2012 01:57 PM, Paolo Carlini wrote:
>> this also triggers the static_assert. Really, in 
>> 'decltype(B{declval<From>()})' almost *everything* is Ok between the 
>> curly brackets. Maybe we should have a separate PR for this.
> And I think this issue is addressed by the ongoing work on 
> instantiation dependency, thus C++/51222 etc.
>
> AFAICS, the most uncertain case is the conditional operator test, 
> otherwise we could split the PR.
I just confirmed that the pending patch for C++/51222 fixes the first 4 
tests.

Paolo.
Paolo Carlini Aug. 8, 2012, 4:35 p.m. UTC | #4
On 08/08/2012 02:47 PM, Paolo Carlini wrote:
> AFAICS, the most uncertain case is the conditional operator test, 
> otherwise we could split the PR.
Turned out to be really trivial: a missing check for error_mark_node in 
build_conditional_expr_1.

I'll send in a separate message a complete regtested patch + testcase 
covering all the tests unrelated to instantiation dependent.

Paolo.
diff mbox

Patch

Index: typeck.c
===================================================================
--- typeck.c	(revision 190207)
+++ typeck.c	(working copy)
@@ -2246,8 +2246,8 @@  build_class_member_access_expr (tree object, tree
 	  tree binfo;
 	  base_kind kind;
 
-	  binfo = lookup_base (access_path ? access_path : object_type,
-			       member_scope, ba_unique,  &kind);
+	  binfo = lookup_base_sfinae (access_path ? access_path : object_type,
+				      member_scope, ba_unique, &kind, complain);
 	  if (binfo == error_mark_node)
 	    return error_mark_node;
 
@@ -2630,7 +2630,8 @@  finish_class_member_access_expr (tree object, tree
 	    }
 
 	  /* Find the base of OBJECT_TYPE corresponding to SCOPE.  */
-	  access_path = lookup_base (object_type, scope, ba_check, NULL);
+	  access_path = lookup_base_sfinae (object_type, scope, ba_check,
+					    NULL, complain);
 	  if (access_path == error_mark_node)
 	    return error_mark_node;
 	  if (!access_path)
@@ -3150,8 +3151,8 @@  get_member_function_from_ptrfunc (tree *instance_p
       if (!same_type_ignoring_top_level_qualifiers_p
 	  (basetype, TREE_TYPE (TREE_TYPE (instance_ptr))))
 	{
-	  basetype = lookup_base (TREE_TYPE (TREE_TYPE (instance_ptr)),
-				  basetype, ba_check, NULL);
+	  basetype = lookup_base_sfinae (TREE_TYPE (TREE_TYPE (instance_ptr)),
+					 basetype, ba_check, NULL, complain);
 	  instance_ptr = build_base_path (PLUS_EXPR, instance_ptr, basetype,
 					  1, complain);
 	  if (instance_ptr == error_mark_node)
@@ -5995,9 +5996,9 @@  build_static_cast_1 (tree type, tree expr, bool c_
 	 ambiguity.  However, if this is a static_cast being performed
 	 because the user wrote a C-style cast, then accessibility is
 	 not considered.  */
-      base = lookup_base (TREE_TYPE (type), intype,
-			  c_cast_p ? ba_unique : ba_check,
-			  NULL);
+      base = lookup_base_sfinae (TREE_TYPE (type), intype,
+				 c_cast_p ? ba_unique : ba_check,
+				 NULL, complain);
 
       /* Convert from "B*" to "D*".  This function will check that "B"
 	 is not a virtual base of "D".  */
@@ -6119,9 +6120,9 @@  build_static_cast_1 (tree type, tree expr, bool c_
 	  && check_for_casting_away_constness (intype, type, STATIC_CAST_EXPR,
 					       complain))
 	return error_mark_node;
-      base = lookup_base (TREE_TYPE (type), TREE_TYPE (intype),
-			  c_cast_p ? ba_unique : ba_check,
-			  NULL);
+      base = lookup_base_sfinae (TREE_TYPE (type), TREE_TYPE (intype),
+				 c_cast_p ? ba_unique : ba_check,
+				 NULL, complain);
       expr = build_base_path (MINUS_EXPR, expr, base, /*nonnull=*/false,
 			      complain);
       return cp_fold_convert(type, expr);
@@ -7181,16 +7182,11 @@  get_delta_difference_1 (tree from, tree to, bool c
 {
   tree binfo;
   base_kind kind;
-  base_access access = c_cast_p ? ba_unique : ba_check;
 
-  /* Note: ba_quiet does not distinguish between access control and
-     ambiguity.  */
-  if (!(complain & tf_error))
-    access |= ba_quiet;
+  binfo = lookup_base_sfinae (to, from, c_cast_p ? ba_unique : ba_check,
+			      &kind, complain);
 
-  binfo = lookup_base (to, from, access, &kind);
-
-  if (kind == bk_inaccessible || kind == bk_ambig)
+  if (binfo == error_mark_node)
     {
       if (!(complain & tf_error))
 	return error_mark_node;
Index: class.c
===================================================================
--- class.c	(revision 190207)
+++ class.c	(working copy)
@@ -274,8 +274,8 @@  build_base_path (enum tree_code code,
 	 a static member function, so we do it now.  */
       if (complain & tf_error)
 	{
-	  tree base = lookup_base (probe, BINFO_TYPE (d_binfo),
-				   ba_unique, NULL);
+	  tree base = lookup_base_sfinae (probe, BINFO_TYPE (d_binfo),
+					  ba_unique, NULL, complain);
 	  gcc_assert (base == error_mark_node);
 	}
       return error_mark_node;
@@ -557,9 +557,8 @@  convert_to_base (tree object, tree type, bool chec
   access = check_access ? ba_check : ba_unique;
   if (!(complain & tf_error))
     access |= ba_quiet;
-  binfo = lookup_base (object_type, type,
-		       access,
-		       NULL);
+  binfo = lookup_base_sfinae (object_type, type,
+			      access, NULL, complain);
   if (!binfo || binfo == error_mark_node)
     return error_mark_node;
 
Index: rtti.c
===================================================================
--- rtti.c	(revision 190207)
+++ rtti.c	(working copy)
@@ -614,8 +614,8 @@  build_dynamic_cast_1 (tree type, tree expr, tsubst
   {
     tree binfo;
 
-    binfo = lookup_base (TREE_TYPE (exprtype), TREE_TYPE (type),
-			 ba_check, NULL);
+    binfo = lookup_base_sfinae (TREE_TYPE (exprtype), TREE_TYPE (type),
+				ba_check, NULL, complain);
 
     if (binfo)
       {
Index: typeck2.c
===================================================================
--- typeck2.c	(revision 190207)
+++ typeck2.c	(working copy)
@@ -1600,7 +1600,7 @@  build_m_component_ref (tree datum, tree component,
     }
   else
     {
-      binfo = lookup_base (objtype, ctype, ba_check, NULL);
+      binfo = lookup_base_sfinae (objtype, ctype, ba_check, NULL, complain);
 
       if (!binfo)
 	{
Index: call.c
===================================================================
--- call.c	(revision 190207)
+++ call.c	(working copy)
@@ -6616,8 +6616,9 @@  build_over_call (struct z_candidate *cand, int fla
       /* If fn was found by a using declaration, the conversion path
 	 will be to the derived class, not the base declaring fn. We
 	 must convert from derived to base.  */
-      base_binfo = lookup_base (TREE_TYPE (TREE_TYPE (converted_arg)),
-				TREE_TYPE (parmtype), ba_unique, NULL);
+      base_binfo = lookup_base_sfinae (TREE_TYPE (TREE_TYPE (converted_arg)),
+				       TREE_TYPE (parmtype), ba_unique,
+				       NULL, complain);
       converted_arg = build_base_path (PLUS_EXPR, converted_arg,
 				       base_binfo, 1, complain);
 
@@ -6859,9 +6860,9 @@  build_over_call (struct z_candidate *cand, int fla
   if (DECL_VINDEX (fn) && (flags & LOOKUP_NONVIRTUAL) == 0)
     {
       tree t;
-      tree binfo = lookup_base (TREE_TYPE (TREE_TYPE (argarray[0])),
-				DECL_CONTEXT (fn),
-				ba_any, NULL);
+      tree binfo = lookup_base_sfinae (TREE_TYPE (TREE_TYPE (argarray[0])),
+				       DECL_CONTEXT (fn),
+				       ba_any, NULL, complain);
       gcc_assert (binfo && binfo != error_mark_node);
 
       /* Warn about deprecated virtual functions now, since we're about
Index: cvt.c
===================================================================
--- cvt.c	(revision 190207)
+++ cvt.c	(working copy)
@@ -149,11 +149,13 @@  cp_convert_to_pointer (tree type, tree expr, tsubs
 	  binfo = NULL_TREE;
 	  /* Try derived to base conversion.  */
 	  if (!same_p)
-	    binfo = lookup_base (intype_class, type_class, ba_check, NULL);
+	    binfo = lookup_base_sfinae (intype_class, type_class, ba_check,
+					NULL, complain);
 	  if (!same_p && !binfo)
 	    {
 	      /* Try base to derived conversion.  */
-	      binfo = lookup_base (type_class, intype_class, ba_check, NULL);
+	      binfo = lookup_base_sfinae (type_class, intype_class, ba_check,
+					  NULL, complain);
 	      code = MINUS_EXPR;
 	    }
 	  if (binfo == error_mark_node)
@@ -278,12 +280,12 @@  convert_to_pointer_force (tree type, tree expr, ts
 	  enum tree_code code = PLUS_EXPR;
 	  tree binfo;
 
-	  binfo = lookup_base (TREE_TYPE (intype), TREE_TYPE (type),
-			       ba_unique, NULL);
+	  binfo = lookup_base_sfinae (TREE_TYPE (intype), TREE_TYPE (type),
+				      ba_unique, NULL, complain);
 	  if (!binfo)
 	    {
-	      binfo = lookup_base (TREE_TYPE (type), TREE_TYPE (intype),
-				   ba_unique, NULL);
+	      binfo = lookup_base_sfinae (TREE_TYPE (type), TREE_TYPE (intype),
+					  ba_unique, NULL, complain);
 	      code = MINUS_EXPR;
 	    }
 	  if (binfo == error_mark_node)
@@ -351,8 +353,9 @@  build_up_reference (tree type, tree arg, int flags
       && MAYBE_CLASS_TYPE_P (argtype)
       && MAYBE_CLASS_TYPE_P (target_type))
     {
-      /* We go through lookup_base for the access control.  */
-      tree binfo = lookup_base (argtype, target_type, ba_check, NULL);
+      /* We go through lookup_base_sfinae for the access control.  */
+      tree binfo = lookup_base_sfinae (argtype, target_type, ba_check,
+				       NULL, complain);
       if (binfo == error_mark_node)
 	return error_mark_node;
       if (binfo == NULL_TREE)
Index: cp-tree.h
===================================================================
--- cp-tree.h	(revision 190207)
+++ cp-tree.h	(working copy)
@@ -5439,6 +5439,8 @@  extern bool emit_tinfo_decl			(tree);
 
 /* in search.c */
 extern bool accessible_base_p			(tree, tree, bool);
+extern tree lookup_base_sfinae			(tree, tree, base_access,
+						 base_kind *, tsubst_flags_t);
 extern tree lookup_base				(tree, tree, base_access,
 						 base_kind *);
 extern tree dcast_base_hint			(tree, tree);
Index: search.c
===================================================================
--- search.c	(revision 190207)
+++ search.c	(working copy)
@@ -185,7 +185,8 @@  accessible_base_p (tree t, tree base, bool conside
    NULL_TREE is returned.  */
 
 tree
-lookup_base (tree t, tree base, base_access access, base_kind *kind_ptr)
+lookup_base_sfinae (tree t, tree base, base_access access,
+		    base_kind *kind_ptr, tsubst_flags_t complain)
 {
   tree binfo;
   tree t_binfo;
@@ -253,7 +254,8 @@  tree
       case bk_ambig:
 	if (!(access & ba_quiet))
 	  {
-	    error ("%qT is an ambiguous base of %qT", base, t);
+	    if (complain & tf_error)
+	      error ("%qT is an ambiguous base of %qT", base, t);
 	    binfo = error_mark_node;
 	  }
 	break;
@@ -271,7 +273,8 @@  tree
 	  {
 	    if (!(access & ba_quiet))
 	      {
-		error ("%qT is an inaccessible base of %qT", base, t);
+		if (complain & tf_error)
+		  error ("%qT is an inaccessible base of %qT", base, t);
 		binfo = error_mark_node;
 	      }
 	    else
@@ -287,6 +290,12 @@  tree
   return binfo;
 }
 
+tree
+lookup_base (tree t, tree base, base_access access, base_kind *kind_ptr)
+{
+  return lookup_base_sfinae (t, base, access, kind_ptr, tf_warning_or_error);
+}
+
 /* Data for dcast_base_hint walker.  */
 
 struct dcast_data_s