Re: [PATCH] c++: fix tf_decltype manipulation for COMPOUND_EXPR

[Patrick Palka <ppalka@redhat.com>]

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

On Fri, 10 Nov 2023, Jason Merrill wrote:

> On 11/10/23 12:25, Patrick Palka wrote:
> > On Thu, 9 Nov 2023, Jason Merrill wrote:
> > 
> > > On 11/7/23 10:08, Patrick Palka wrote:
> > > > bootstrapped and regtested on x86_64-pc-linxu-gnu, does this look OK for
> > > > trunk?
> > > > 
> > > > -- >8 --
> > > > 
> > > > In the COMPOUND_EXPR case of tsubst_expr, we were redundantly clearing
> > > > the tf_decltype flag when substituting the LHS and also neglecting to
> > > > propagate it when substituting the RHS.  This patch corrects this flag
> > > > manipulation, which allows us to accept the below testcase.
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* pt.cc (tsubst_expr) <case COMPOUND_EXPR>: Don't redundantly
> > > > 	clear tf_decltype when substituting the LHS.  Propagate
> > > > 	tf_decltype when substituting the RHS.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	* g++.dg/cpp0x/decltype-call7.C: New test.
> > > > ---
> > > >    gcc/cp/pt.cc                                | 9 ++++-----
> > > >    gcc/testsuite/g++.dg/cpp0x/decltype-call7.C | 9 +++++++++
> > > >    2 files changed, 13 insertions(+), 5 deletions(-)
> > > >    create mode 100644 gcc/testsuite/g++.dg/cpp0x/decltype-call7.C
> > > > 
> > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > > > index 521749df525..5f879287a58 100644
> > > > --- a/gcc/cp/pt.cc
> > > > +++ b/gcc/cp/pt.cc
> > > > @@ -20382,11 +20382,10 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> > > > complain, tree in_decl)
> > > >          case COMPOUND_EXPR:
> > > >          {
> > > > -	tree op0 = tsubst_expr (TREE_OPERAND (t, 0), args,
> > > > -				complain & ~tf_decltype, in_decl);
> > > > -	RETURN (build_x_compound_expr (EXPR_LOCATION (t),
> > > > -				       op0,
> > > > -				       RECUR (TREE_OPERAND (t, 1)),
> > > > +	tree op0 = RECUR (TREE_OPERAND (t, 0));
> > > > +	tree op1 = tsubst_expr (TREE_OPERAND (t, 1), args,
> > > > +				complain|decltype_flag, in_decl);
> > > > +	RETURN (build_x_compound_expr (EXPR_LOCATION (t), op0, op1,
> > > >    				       templated_operator_saved_lookups
> > > > (t),
> > > >    				       complain|decltype_flag));
> > > 
> > > Hmm, passing decltype_flag to both op1 and the , is concerning.  Can you
> > > add a
> > > test with overloaded operator, where the RHS is a class with a destructor?
> > 
> > I'm not sure if this is what you had in mind, but indeed with this patch
> > we reject the following with an error outside the immediate context:
> > 
> >      struct B { ~B() = delete; };
> >      template<class T> B f();
> > 
> >      void operator,(int, const B&);
> > 
> >      template<class T> decltype(42, f<T>()) g(int) = delete; // #1
> >      template<class T> void g(...); // #2
> > 
> >      int main() {
> >        g<B>(0); // should select #2
> >      }
> > 
> > gcc/testsuite/g++.dg/cpp0x/decltype-call8.C: In substitution of
> > ‘template<class T> decltype ((42, f<T>())) g(int) [with T = B]’:
> > gcc/testsuite/g++.dg/cpp0x/decltype-call8.C:12:7:   required from here
> >     12 |   g<B>(0);
> >        |   ~~~~^~~
> > gcc/testsuite/g++.dg/cpp0x/decltype-call8.C:8:30: error: use of deleted
> > function ‘B::~B()’
> >      8 | template<class T> decltype(42, f<T>()) g(int) = delete; // #1
> >        |                            ~~^~~~~~~~
> > gcc/testsuite/g++.dg/cpp0x/decltype-call8.C:3:12: note: declared here
> >      3 | struct B { ~B() = delete; };
> >        |            ^
> > 
> > Ultimately because unary_complex_lvalue isn't SFINAE-enabled.
> 
> Please elaborate; my understanding is that unary_complex_lvalue is supposed to
> be a semantically neutral transformation.

Since tf_decltype is now also set when substituting op1 i.e. f<T>(),
substitution yields a bare CALL_EXPR with no temporary materialization.
The problematic unary_complex_lvalue call happens when binding
the reference parameter 'const B&' to this bare CALL_EXPR.  We
take its address via cp_build_addr_expr, which tries
unary_complex_lvalue.  The CALL_EXPR handling in unary_complex_lvalue
in turn materializes a temporary for the call, which fails as expected
due to the destructor but also issues the unexpected error since
unary_complex_lvalue unconditionally uses tf_warning_or_error.

So in short the CALL_EXPR handling in unary_complex_lvalue seems to
assume the requirements of temporary materialization have already
been checked.

> 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 4f2cb2cd402..277c81412b9 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -20386,7 +20386,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> > complain, tree in_decl)
> >   				complain|decltype_flag, in_decl);
> >   	RETURN (build_x_compound_expr (EXPR_LOCATION (t), op0, op1,
> >   				       templated_operator_saved_lookups (t),
> > -				       complain|decltype_flag));
> > +				       complain));
> 
> This looks like it will break if the operator, returns a class with a deleted
> destructor.

Ah sorry about that, this hunk wasn't intended to be part of the diff.
Here's a full tested patch for consideration.  The additional
build_const_cast_1 call in cp_build_c_cast is needed to preserve the
last four errors in gcc/testsuite/g++.dg/tree-ssa/pr20280.C (which get
indirectly issued from unary_complex_lvalue).

-- >8 --

Subject: [PATCH] c++: make unary_complex_lvalue SFINAE-enabled

gcc/cp/ChangeLog:

	* class.cc (build_simple_base_path): Adjust call to
	unary_complex_value.
	* cp-tree.h (unary_complex_lvalue): Add complain parameter.
	* typeck.cc (build_class_member_access_expr): Adjust call to
	unary_complex_lvalue.
	(cp_build_addr_expr_1): Likewise.
	(cp_build_unary_op): Likewise.
	(unary_complex_lvalue): Add complain parameter and propagate
	it.  Propagate error_mark_node result from build_cplus_new.
	(cp_build_c_cast): If we're committed to a const_cast and
	the result is erroneous, call build_const_cast_1 a second time
	to issue errors.
---
 gcc/cp/class.cc  |  2 +-
 gcc/cp/cp-tree.h |  2 +-
 gcc/cp/typeck.cc | 27 +++++++++++++++++----------
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 9d4d95f85bf..58c45542793 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -556,7 +556,7 @@ build_simple_base_path (tree expr, tree binfo)
 	 into `(*(a ?  &b : &c)).x', and so on.  A COND_EXPR is only
 	 an lvalue in the front end; only _DECLs and _REFs are lvalues
 	 in the back end.  */
-      temp = unary_complex_lvalue (ADDR_EXPR, expr);
+      temp = unary_complex_lvalue (ADDR_EXPR, expr, tf_warning_or_error);
       if (temp)
 	expr = cp_build_fold_indirect_ref (temp);
 
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 1fa710d7154..d826afcdb5c 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -8130,7 +8130,7 @@ extern tree cp_build_addr_expr			(tree, tsubst_flags_t);
 extern tree cp_build_unary_op                   (enum tree_code, tree, bool,
                                                  tsubst_flags_t);
 extern tree genericize_compound_lvalue		(tree);
-extern tree unary_complex_lvalue		(enum tree_code, tree);
+extern tree unary_complex_lvalue		(enum tree_code, tree, tsubst_flags_t);
 extern tree build_x_conditional_expr		(location_t, tree, tree, tree,
                                                  tsubst_flags_t);
 extern tree build_x_compound_expr_from_list	(tree, expr_list_kind,
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 49afbd8fb5e..be6667c192e 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -2888,7 +2888,7 @@ build_class_member_access_expr (cp_expr object, tree member,
   /* Transform `(a, b).x' into `(*(a, &b)).x', `(a ? b : c).x' into
      `(*(a ?  &b : &c)).x', and so on.  A COND_EXPR is only an lvalue
      in the front end; only _DECLs and _REFs are lvalues in the back end.  */
-  if (tree temp = unary_complex_lvalue (ADDR_EXPR, object))
+  if (tree temp = unary_complex_lvalue (ADDR_EXPR, object, complain))
     {
       temp = cp_build_fold_indirect_ref (temp);
       if (!lvalue_p (object) && lvalue_p (temp))
@@ -7118,7 +7118,7 @@ cp_build_addr_expr_1 (tree arg, bool strict_lvalue, tsubst_flags_t complain)
 
   /* Handle complex lvalues (when permitted)
      by reduction to simpler cases.  */
-  val = unary_complex_lvalue (ADDR_EXPR, arg);
+  val = unary_complex_lvalue (ADDR_EXPR, arg, complain);
   if (val != 0)
     return val;
 
@@ -7411,7 +7411,7 @@ cp_build_unary_op (enum tree_code code, tree xarg, bool noconvert,
       /* Handle complex lvalues (when permitted)
 	 by reduction to simpler cases.  */
 
-      val = unary_complex_lvalue (code, arg);
+      val = unary_complex_lvalue (code, arg, complain);
       if (val != 0)
 	goto return_build_unary_op;
 
@@ -7657,7 +7657,7 @@ genericize_compound_lvalue (tree lvalue)
    NULL_TREE.  */
 
 tree
-unary_complex_lvalue (enum tree_code code, tree arg)
+unary_complex_lvalue (enum tree_code code, tree arg, tsubst_flags_t complain)
 {
   /* Inside a template, making these kinds of adjustments is
      pointless; we are only concerned with the type of the
@@ -7669,7 +7669,7 @@ unary_complex_lvalue (enum tree_code code, tree arg)
   if (TREE_CODE (arg) == COMPOUND_EXPR)
     {
       tree real_result = cp_build_unary_op (code, TREE_OPERAND (arg, 1), false,
-                                            tf_warning_or_error);
+					    complain);
       return build2 (COMPOUND_EXPR, TREE_TYPE (real_result),
 		     TREE_OPERAND (arg, 0), real_result);
     }
@@ -7677,13 +7677,14 @@ unary_complex_lvalue (enum tree_code code, tree arg)
   /* Handle (a ? b : c) used as an "lvalue".  */
   if (TREE_CODE (arg) == COND_EXPR
       || TREE_CODE (arg) == MIN_EXPR || TREE_CODE (arg) == MAX_EXPR)
-    return rationalize_conditional_expr (code, arg, tf_warning_or_error);
+    return rationalize_conditional_expr (code, arg, complain);
 
   /* Handle (a = b), (++a), and (--a) used as an "lvalue".  */
   if (TREE_CODE (arg) == MODIFY_EXPR
       || TREE_CODE (arg) == PREINCREMENT_EXPR
       || TREE_CODE (arg) == PREDECREMENT_EXPR)
-    return unary_complex_lvalue (code, genericize_compound_lvalue (arg));
+    return unary_complex_lvalue (code, genericize_compound_lvalue (arg),
+				 complain);
 
   if (code != ADDR_EXPR)
     return NULL_TREE;
@@ -7693,7 +7694,7 @@ unary_complex_lvalue (enum tree_code code, tree arg)
       || TREE_CODE (arg) == INIT_EXPR)
     {
       tree real_result = cp_build_unary_op (code, TREE_OPERAND (arg, 0), false,
-                                            tf_warning_or_error);
+					    complain);
       arg = build2 (COMPOUND_EXPR, TREE_TYPE (real_result),
 		    arg, real_result);
       suppress_warning (arg /* What warning? */);
@@ -7717,7 +7718,11 @@ unary_complex_lvalue (enum tree_code code, tree arg)
 	if (TREE_CODE (arg) == SAVE_EXPR)
 	  targ = arg;
 	else
-	  targ = build_cplus_new (TREE_TYPE (arg), arg, tf_warning_or_error);
+	  {
+	    targ = build_cplus_new (TREE_TYPE (arg), arg, complain);
+	    if (targ == error_mark_node)
+	      return error_mark_node;
+	  }
 	return build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (arg)), targ);
       }
 
@@ -9174,6 +9179,8 @@ cp_build_c_cast (location_t loc, tree type, tree expr,
 	  maybe_warn_about_useless_cast (loc, type, value, complain);
 	  maybe_warn_about_cast_ignoring_quals (loc, type, complain);
 	}
+      else if (complain & tf_error)
+	build_const_cast_1 (loc, type, value, tf_error, &valid_p);
       return result;
     }
 
@@ -9209,7 +9216,7 @@ cp_build_c_cast (location_t loc, tree type, tree expr,
 	 to succeed.  */
       if (!same_type_p (non_reference (type), non_reference (result_type)))
 	{
-	  result = build_const_cast_1 (loc, type, result, false, &valid_p);
+	  result = build_const_cast_1 (loc, type, result, tf_none, &valid_p);
 	  gcc_assert (valid_p);
 	}
       return result;
-- 
2.43.0.rc1