[C++,PR84231] overload on cond_expr in template

Message ID orvaf6gbhl.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [C++,PR84231] overload on cond_expr in template
Related show

Commit Message

Alexandre Oliva Feb. 9, 2018, 2:09 a.m.
A non-type-dependent COND_EXPR within a template is reconstructed with
the original operands, after one with non-dependent proxies is built to
determine its result type.  This is problematic because the operands of
a COND_EXPR determined to be an rvalue may have been converted to denote
their rvalue nature.  The reconstructed one, however, won't have such
conversions, so lvalue_kind may not recognize it as an rvalue, which may
lead to e.g. incorrect overload resolution decisions.

If we mistake such a COND_EXPR for an lvalue, overload resolution might
regard a conversion sequence that binds it to a non-const reference as
viable, and then select that over one that binds it to a const
reference.  Only after template substitution would we rebuild the
COND_EXPR, realize it is an rvalue, and conclude the reference binding
is ill-formed, but at that point we'd have long discarded any alternate
candidates we could have used.

This patch verifies that, if a non-type-dependent COND_EXPRs is
recognized as an rvalues, so is the to-be-template-substituted one
created in its stead, so that overload resolution selects the correct
alternative.

Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?

for  gcc/cp/ChangeLog

	PR c++/84231
	* typeck.c (build_x_conditional_expr): Make sure the
	to-be-tsubsted expr is an rvalue when it should.

for  gcc/testsuite/g++.dg/ChangeLog

	PR c++/84231
	* pr84231.C: New.
---
 gcc/cp/typeck.c                |   16 +++++++++++++++-
 gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/pr84231.C


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Comments

Jason Merrill Feb. 15, 2018, 7:20 p.m. | #1
On Thu, Feb 8, 2018 at 9:09 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> +         /* If it was supposed to be an rvalue but it's not, adjust

> +            one of the operands so that any overload resolution

> +            taking this COND_EXPR as an operand makes the correct

> +            decisions.  See c++/84231.  */

> +         TREE_OPERAND (min, 2) = build1_loc (loc, NON_LVALUE_EXPR,

> +                                             TREE_TYPE (min),

> +                                             TREE_OPERAND (min, 2));

> +         EXPR_LOCATION_WRAPPER_P (TREE_OPERAND (min, 2)) = 1;


But that's not true, this isn't a location wrapper, it has semantic
effect.  And would be the first such use of NON_LVALUE_EXPR in a
template.

Since we're already using the type of the COND_EXPR to indicate a
glvalue, maybe lvalue_kind should say that within a template, a
COND_EXPR which got past the early check for reference type is a
prvalue.

Jason
Alexandre Oliva Feb. 27, 2018, 6:05 p.m. | #2
On Feb 15, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Thu, Feb 8, 2018 at 9:09 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> +         /* If it was supposed to be an rvalue but it's not, adjust

>> +            one of the operands so that any overload resolution

>> +            taking this COND_EXPR as an operand makes the correct

>> +            decisions.  See c++/84231.  */

>> +         TREE_OPERAND (min, 2) = build1_loc (loc, NON_LVALUE_EXPR,

>> +                                             TREE_TYPE (min),

>> +                                             TREE_OPERAND (min, 2));

>> +         EXPR_LOCATION_WRAPPER_P (TREE_OPERAND (min, 2)) = 1;


> But that's not true, this isn't a location wrapper, it has semantic

> effect.  And would be the first such use of NON_LVALUE_EXPR in a

> template.


Yeah.  At first I thought NON_LVALUE_EXPR was the way to go, as the
traditional way to denote non-lvalues, but when that didn't work, I
investigated and saw if I marked it as a location wrapper, it would have
the intended effect of stopping the template-dependent cond_expr from
being regarded as an lvalue, while being dropped when tsubsting the
cond_expr, so it had no ill effects AFAICT.

> Since we're already using the type of the COND_EXPR to indicate a

> glvalue, maybe lvalue_kind should say that within a template, a

> COND_EXPR which got past the early check for reference type is a

> prvalue.


I suppose you mean something like this:

diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 9b9e36a1173f..76148c876b71 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -194,6 +194,14 @@ lvalue_kind (const_tree ref)
       break;
 
     case COND_EXPR:
+      /* Except for type-dependent exprs, a REFERENCE_TYPE will
+	 indicate whether its result is an lvalue or so.
+	 REFERENCE_TYPEs are handled above, so if we reach this point,
+	 we know we got an rvalue, unless we have a type-dependent
+	 expr.  */
+      if (processing_template_decl
+	  && !type_dependent_expression_p (CONST_CAST_TREE (ref)))
+	return clk_none;
       op1_lvalue_kind = lvalue_kind (TREE_OPERAND (ref, 1)
 				    ? TREE_OPERAND (ref, 1)
 				    : TREE_OPERAND (ref, 0));

but there be dragons here.  build_x_conditional_expr wants tests
glvalue_p on the proxy and the template expr, and glvalue_p uses
lvalue_kind, so we have to disable this new piece of logic for the
baseline so that we don't unintentionally change the lvalueness of the
COND_EXPR.

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 0e7c63dd1973..a34cb6ec175f 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -6565,11 +6565,25 @@ build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,
     {
       tree min = build_min_non_dep (COND_EXPR, expr,
 				    orig_ifexp, orig_op1, orig_op2);
-      /* Remember that the result is an lvalue or xvalue.  */
-      if (glvalue_p (expr) && !glvalue_p (min))
-	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),
-						   !lvalue_p (expr));
+      /* Remember that the result is an lvalue or xvalue.  We have to
+	 pretend EXPR is type-dependent, lest we short-circuit the
+	 very logic we want to rely on.  */
+      tree save_expr_type = TREE_TYPE (expr);
+
+      if (!type_dependent_expression_p (expr)
+	  && TREE_CODE (save_expr_type) != REFERENCE_TYPE)
+	TREE_TYPE (expr) = NULL_TREE;
+      
+      bool glvalue = glvalue_p (expr);
+      bool reftype = glvalue && !glvalue_p (min);
+      bool lval = reftype ? lvalue_p (expr) : false;
+
+      TREE_TYPE (expr) = save_expr_type;
+
+      if (reftype)
+	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min), !lval);
       expr = convert_from_reference (min);
+      gcc_assert (glvalue_p (min) == glvalue);
     }
   return expr;
 }


Even then, there are other surprises I'm trying to track down (libstdc++
optimized headers won't build with the two patchlets above); my guess is
that it's out of non-template-dependent cond_exprs' transitions from
non-lvalue to lvalue as we finish template substitution and
processing_template_decl becomes zero.

This is getting hairy enough that I'm wondering if that's really what
you had in mind, so I decided to touch base in case I had to be put back
on the right track (or rather out of the wrong track again ;-)

Thanks,

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill Feb. 27, 2018, 6:21 p.m. | #3
On Tue, Feb 27, 2018 at 1:05 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 15, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Thu, Feb 8, 2018 at 9:09 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> +         /* If it was supposed to be an rvalue but it's not, adjust

>>> +            one of the operands so that any overload resolution

>>> +            taking this COND_EXPR as an operand makes the correct

>>> +            decisions.  See c++/84231.  */

>>> +         TREE_OPERAND (min, 2) = build1_loc (loc, NON_LVALUE_EXPR,

>>> +                                             TREE_TYPE (min),

>>> +                                             TREE_OPERAND (min, 2));

>>> +         EXPR_LOCATION_WRAPPER_P (TREE_OPERAND (min, 2)) = 1;

>

>> But that's not true, this isn't a location wrapper, it has semantic

>> effect.  And would be the first such use of NON_LVALUE_EXPR in a

>> template.

>

> Yeah.  At first I thought NON_LVALUE_EXPR was the way to go, as the

> traditional way to denote non-lvalues, but when that didn't work, I

> investigated and saw if I marked it as a location wrapper, it would have

> the intended effect of stopping the template-dependent cond_expr from

> being regarded as an lvalue, while being dropped when tsubsting the

> cond_expr, so it had no ill effects AFAICT.

>

>> Since we're already using the type of the COND_EXPR to indicate a

>> glvalue, maybe lvalue_kind should say that within a template, a

>> COND_EXPR which got past the early check for reference type is a

>> prvalue.

>

> I suppose you mean something like this:

>

> diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c

> index 9b9e36a1173f..76148c876b71 100644

> --- a/gcc/cp/tree.c

> +++ b/gcc/cp/tree.c

> @@ -194,6 +194,14 @@ lvalue_kind (const_tree ref)

>        break;

>

>      case COND_EXPR:

> +      /* Except for type-dependent exprs, a REFERENCE_TYPE will

> +        indicate whether its result is an lvalue or so.

> +        REFERENCE_TYPEs are handled above, so if we reach this point,

> +        we know we got an rvalue, unless we have a type-dependent

> +        expr.  */

> +      if (processing_template_decl

> +         && !type_dependent_expression_p (CONST_CAST_TREE (ref)))

> +       return clk_none;

>        op1_lvalue_kind = lvalue_kind (TREE_OPERAND (ref, 1)

>                                     ? TREE_OPERAND (ref, 1)

>                                     : TREE_OPERAND (ref, 0));

>

> but there be dragons here.  build_x_conditional_expr wants tests

> glvalue_p on the proxy and the template expr, and glvalue_p uses

> lvalue_kind, so we have to disable this new piece of logic for the

> baseline so that we don't unintentionally change the lvalueness of the

> COND_EXPR.

>

> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c

> index 0e7c63dd1973..a34cb6ec175f 100644

> --- a/gcc/cp/typeck.c

> +++ b/gcc/cp/typeck.c

> @@ -6565,11 +6565,25 @@ build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,

>      {

>        tree min = build_min_non_dep (COND_EXPR, expr,

>                                     orig_ifexp, orig_op1, orig_op2);

> -      /* Remember that the result is an lvalue or xvalue.  */

> -      if (glvalue_p (expr) && !glvalue_p (min))

> -       TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),

> -                                                  !lvalue_p (expr));

> +      /* Remember that the result is an lvalue or xvalue.  We have to

> +        pretend EXPR is type-dependent, lest we short-circuit the

> +        very logic we want to rely on.  */

> +      tree save_expr_type = TREE_TYPE (expr);

> +

> +      if (!type_dependent_expression_p (expr)

> +         && TREE_CODE (save_expr_type) != REFERENCE_TYPE)

> +       TREE_TYPE (expr) = NULL_TREE;

> +

> +      bool glvalue = glvalue_p (expr);

> +      bool reftype = glvalue && !glvalue_p (min);

> +      bool lval = reftype ? lvalue_p (expr) : false;

> +

> +      TREE_TYPE (expr) = save_expr_type;

> +

> +      if (reftype)

> +       TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min), !lval);

>        expr = convert_from_reference (min);

> +      gcc_assert (glvalue_p (min) == glvalue);

>      }

>    return expr;

>  }

>

>

> Even then, there are other surprises I'm trying to track down (libstdc++

> optimized headers won't build with the two patchlets above); my guess is

> that it's out of non-template-dependent cond_exprs' transitions from

> non-lvalue to lvalue as we finish template substitution and

> processing_template_decl becomes zero.

>

> This is getting hairy enough that I'm wondering if that's really what

> you had in mind, so I decided to touch base in case I had to be put back

> on the right track (or rather out of the wrong track again ;-)


Perhaps it would be easier to add the REFERENCE_TYPE in
build_conditional_expr_1, adjusting result_type based on
processing_template_decl and is_lvalue.

Jason
Alexandre Oliva Feb. 28, 2018, 5:24 a.m. | #4
On Feb 27, 2018, Jason Merrill <jason@redhat.com> wrote:

> Perhaps it would be easier to add the REFERENCE_TYPE in

> build_conditional_expr_1, adjusting result_type based on

> processing_template_decl and is_lvalue.


It is, indeed!

Here's the patch, regstrapped on i686- and x86_64-linux-gnu.  The only
unexpected glitch was the need for adjusting the fold expr parser to
deal with an indirect_ref, lest g++.dg/cpp1x/fold6.C would fail to
error at the line with the ternary operator.

Ok to install?


[C++] [PR84231] overload on cond_expr in template

A non-type-dependent COND_EXPR within a template is reconstructed with
the original operands, after one with non-dependent proxies is built to
determine its result type.  This is problematic because the operands of
a COND_EXPR determined to be an rvalue may have been converted to denote
their rvalue nature.  The reconstructed one, however, won't have such
conversions, so lvalue_kind may not recognize it as an rvalue, which may
lead to e.g. incorrect overload resolution decisions.

If we mistake such a COND_EXPR for an lvalue, overload resolution might
regard a conversion sequence that binds it to a non-const reference as
viable, and then select that over one that binds it to a const
reference.  Only after template substitution would we rebuild the
COND_EXPR, realize it is an rvalue, and conclude the reference binding
is ill-formed, but at that point we'd have long discarded any alternate
candidates we could have used.

This patch modifies the logic that determines whether a
(non-type-dependent) COND_EXPR in a template is an lvalue, to rely on
its type, more specifically, on the presence of a REFERENCE_TYPE
wrapper.  In order to avoid a type bootstrapping problem, the
REFERENCE_TYPE that wraps the type of some such COND_EXPRs is
introduced earlier, so that we don't have to test for lvalueness of
the expression using the very code that we wish to change.


for  gcc/cp/ChangeLog

	PR c++/84231
	* tree.c (lvalue_kind): Use presence/absence of REFERENCE_TYPE
	only while processing template decls.
	* typeck.c (build_x_conditional_expr): Move wrapping of
	reference type around type...
	* call.c (build_conditional_expr_1): ... here.
	* parser.c (cp_parser_fold_expression): Catch REFERENCE_REF_P
	INDIRECT_REF of COND_EXPR too.

for  gcc/testsuite/ChangeLog

	PR c++/84231
	* g++.dg/pr84231.C: New.
---
 gcc/cp/call.c                  |    3 +++
 gcc/cp/parser.c                |    4 +++-
 gcc/cp/tree.c                  |    8 ++++++++
 gcc/cp/typeck.c                |    4 ----
 gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr84231.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 11fe28292fb1..9d98a3d90d25 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5348,6 +5348,9 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
     return error_mark_node;
 
  valid_operands:
+  if (processing_template_decl)
+    result_type = cp_build_reference_type (result_type, !is_lvalue);
+
   result = build3_loc (loc, COND_EXPR, result_type, arg1, arg2, arg3);
 
   /* If the ARG2 and ARG3 are the same and don't have side-effects,
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index bcee1214c2f3..c483b6ce25ea 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -4961,7 +4961,9 @@ cp_parser_fold_expression (cp_parser *parser, tree expr1)
   else if (is_binary_op (TREE_CODE (expr1)))
     error_at (location_of (expr1),
 	      "binary expression in operand of fold-expression");
-  else if (TREE_CODE (expr1) == COND_EXPR)
+  else if (TREE_CODE (expr1) == COND_EXPR
+	   || (REFERENCE_REF_P (expr1)
+	       && TREE_CODE (TREE_OPERAND (expr1, 0)) == COND_EXPR))
     error_at (location_of (expr1),
 	      "conditional expression in operand of fold-expression");
 
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 9b9e36a1173f..76148c876b71 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -194,6 +194,14 @@ lvalue_kind (const_tree ref)
       break;
 
     case COND_EXPR:
+      /* Except for type-dependent exprs, a REFERENCE_TYPE will
+	 indicate whether its result is an lvalue or so.
+	 REFERENCE_TYPEs are handled above, so if we reach this point,
+	 we know we got an rvalue, unless we have a type-dependent
+	 expr.  */
+      if (processing_template_decl
+	  && !type_dependent_expression_p (CONST_CAST_TREE (ref)))
+	return clk_none;
       op1_lvalue_kind = lvalue_kind (TREE_OPERAND (ref, 1)
 				    ? TREE_OPERAND (ref, 1)
 				    : TREE_OPERAND (ref, 0));
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 0e7c63dd1973..fba04c49ec2d 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -6565,10 +6565,6 @@ build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,
     {
       tree min = build_min_non_dep (COND_EXPR, expr,
 				    orig_ifexp, orig_op1, orig_op2);
-      /* Remember that the result is an lvalue or xvalue.  */
-      if (glvalue_p (expr) && !glvalue_p (min))
-	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),
-						   !lvalue_p (expr));
       expr = convert_from_reference (min);
     }
   return expr;
diff --git a/gcc/testsuite/g++.dg/pr84231.C b/gcc/testsuite/g++.dg/pr84231.C
new file mode 100644
index 000000000000..de7c89a2ab69
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr84231.C
@@ -0,0 +1,29 @@
+// PR c++/84231 - overload resolution with cond_expr in a template
+
+// { dg-do compile }
+
+struct format {
+  template<typename T> format& operator%(const T&) { return *this; }
+  template<typename T> format& operator%(T&) { return *this; }
+};
+
+format f;
+
+template <typename>
+void function_template(bool b)
+{
+  // Compiles OK with array lvalue:
+  f % (b ? "x" : "x");
+
+  // Used to fails with pointer rvalue:
+  f % (b ? "" : "x");
+}
+
+void normal_function(bool b)
+{
+  // Both cases compile OK in non-template function:
+  f % (b ? "x" : "x");
+  f % (b ? "" : "x");
+
+  function_template<void>(b);
+}



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill Feb. 28, 2018, 4:51 p.m. | #5
On Wed, Feb 28, 2018 at 12:24 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> +  if (processing_template_decl)

> +    result_type = cp_build_reference_type (result_type, !is_lvalue);


If !is_lvalue, we don't want a reference type at all, as the result is
a prvalue.  is_lvalue should probably rename to is_glvalue.

The second argument to cp_build_reference_type should be xvalue_p (arg2).

> +      /* Except for type-dependent exprs, a REFERENCE_TYPE will

> +        indicate whether its result is an lvalue or so.


"or not" ?

> +      if (processing_template_decl

> +         && !type_dependent_expression_p (CONST_CAST_TREE (ref)))

> +       return clk_none;


I think we want to return clk_class for a COND_EXPR of class type.

Can we actually get here for a type-dependent expression?  I'd think
we'd never get as far as asking whether a type-dependent expression is
an lvalue, since in general we can't answer that question.

Jason
Alexandre Oliva March 2, 2018, 7:57 a.m. | #6
On Feb 28, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Wed, Feb 28, 2018 at 12:24 AM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> +  if (processing_template_decl)

>> +    result_type = cp_build_reference_type (result_type, !is_lvalue);


> If !is_lvalue, we don't want a reference type at all, as the result is

> a prvalue.  is_lvalue should probably rename to is_glvalue.


I ended up moving the above to the path in which we deal with lvalues
and xvalues.  I still renamed the variable as suggested, though I no
longer use it.

> The second argument to cp_build_reference_type should be xvalue_p (arg2).


I thought of adding a comment as to why testing just arg2 was correct,
but then moving the code kind of made it obvious, didn't it?

>> +      /* Except for type-dependent exprs, a REFERENCE_TYPE will

>> +        indicate whether its result is an lvalue or so.


> "or not" ?


I meant "or so" as in "or other kinds of reference values".

>> +      if (processing_template_decl

>> +         && !type_dependent_expression_p (CONST_CAST_TREE (ref)))

>> +       return clk_none;


> I think we want to return clk_class for a COND_EXPR of class type.


or array, like the default case, I suppose.

> Can we actually get here for a type-dependent expression?  I'd think

> we'd never get as far as asking whether a type-dependent expression is

> an lvalue, since in general we can't answer that question.


We shouldn't, indeed.  And AFAICT we don't; I've added an assert to make
sure.

[C++] [PR84231] overload on cond_expr in template

A non-type-dependent COND_EXPR within a template is reconstructed with
the original operands, after one with non-dependent proxies is built to
determine its result type.  This is problematic because the operands of
a COND_EXPR determined to be an rvalue may have been converted to denote
their rvalue nature.  The reconstructed one, however, won't have such
conversions, so lvalue_kind may not recognize it as an rvalue, which may
lead to e.g. incorrect overload resolution decisions.

If we mistake such a COND_EXPR for an lvalue, overload resolution might
regard a conversion sequence that binds it to a non-const reference as
viable, and then select that over one that binds it to a const
reference.  Only after template substitution would we rebuild the
COND_EXPR, realize it is an rvalue, and conclude the reference binding
is ill-formed, but at that point we'd have long discarded any alternate
candidates we could have used.

This patch modifies the logic that determines whether a
(non-type-dependent) COND_EXPR in a template is an lvalue, to rely on
its type, more specifically, on the presence of a REFERENCE_TYPE
wrapper.  In order to avoid a type bootstrapping problem, the
REFERENCE_TYPE that wraps the type of some such COND_EXPRs is
introduced earlier, so that we don't have to test for lvalueness of
the expression using the very code that we wish to change.


for  gcc/cp/ChangeLog

	PR c++/84231
	* tree.c (lvalue_kind): Use presence/absence of REFERENCE_TYPE
	only while processing template decls.
	* typeck.c (build_x_conditional_expr): Move wrapping of
	reference type around type...
	* call.c (build_conditional_expr_1): ... here.  Rename
	is_lvalue to is_glvalue.
	* parser.c (cp_parser_fold_expression): Catch REFERENCE_REF_P
	INDIRECT_REF of COND_EXPR too.

for  gcc/testsuite/ChangeLog

	PR c++/84231
	* g++.dg/pr84231.C: New.
---
 gcc/cp/call.c                  |   11 +++++++----
 gcc/cp/parser.c                |    4 +++-
 gcc/cp/tree.c                  |   15 +++++++++++++++
 gcc/cp/typeck.c                |    4 ----
 gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr84231.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 11fe28292fb1..6707aa2d3f02 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4782,7 +4782,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
   tree arg3_type;
   tree result = NULL_TREE;
   tree result_type = NULL_TREE;
-  bool is_lvalue = true;
+  bool is_glvalue = true;
   struct z_candidate *candidates = 0;
   struct z_candidate *cand;
   void *p;
@@ -5037,7 +5037,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
 	  return error_mark_node;
 	}
 
-      is_lvalue = false;
+      is_glvalue = false;
       goto valid_operands;
     }
   /* [expr.cond]
@@ -5155,6 +5155,9 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
       && same_type_p (arg2_type, arg3_type))
     {
       result_type = arg2_type;
+      if (processing_template_decl)
+	result_type = cp_build_reference_type (result_type, xvalue_p (arg2));
+
       arg2 = mark_lvalue_use (arg2);
       arg3 = mark_lvalue_use (arg3);
       goto valid_operands;
@@ -5167,7 +5170,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
      cv-qualified) class type, overload resolution is used to
      determine the conversions (if any) to be applied to the operands
      (_over.match.oper_, _over.built_).  */
-  is_lvalue = false;
+  is_glvalue = false;
   if (!same_type_p (arg2_type, arg3_type)
       && (CLASS_TYPE_P (arg2_type) || CLASS_TYPE_P (arg3_type)))
     {
@@ -5361,7 +5364,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
   /* We can't use result_type below, as fold might have returned a
      throw_expr.  */
 
-  if (!is_lvalue)
+  if (!is_glvalue)
     {
       /* Expand both sides into the same slot, hopefully the target of
 	 the ?: expression.  We used to check for TARGET_EXPRs here,
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index e1acb07d29ef..f21257f41e7b 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -4963,7 +4963,9 @@ cp_parser_fold_expression (cp_parser *parser, tree expr1)
   else if (is_binary_op (TREE_CODE (expr1)))
     error_at (location_of (expr1),
 	      "binary expression in operand of fold-expression");
-  else if (TREE_CODE (expr1) == COND_EXPR)
+  else if (TREE_CODE (expr1) == COND_EXPR
+	   || (REFERENCE_REF_P (expr1)
+	       && TREE_CODE (TREE_OPERAND (expr1, 0)) == COND_EXPR))
     error_at (location_of (expr1),
 	      "conditional expression in operand of fold-expression");
 
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 19f1c0629c9a..4cf2126608f0 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -194,6 +194,21 @@ lvalue_kind (const_tree ref)
       break;
 
     case COND_EXPR:
+      if (processing_template_decl)
+	{
+	  /* Within templates, a REFERENCE_TYPE will indicate whether
+	     the COND_EXPR result is an ordinary lvalue or rvalueref.
+	     Since REFERENCE_TYPEs are handled above, if we reach this
+	     point, we know we got a plain rvalue.  Unless we have a
+	     type-dependent expr, that is, but we shouldn't be testing
+	     lvalueness if we can't even tell the types yet!  */
+	  gcc_assert (!type_dependent_expression_p (CONST_CAST_TREE (ref)));
+	  if (CLASS_TYPE_P (TREE_TYPE (ref))
+	      || TREE_CODE (TREE_TYPE (ref)) == ARRAY_TYPE)
+	    return clk_class;
+	  else
+	    return clk_none;
+	}
       op1_lvalue_kind = lvalue_kind (TREE_OPERAND (ref, 1)
 				    ? TREE_OPERAND (ref, 1)
 				    : TREE_OPERAND (ref, 0));
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 0e7c63dd1973..fba04c49ec2d 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -6565,10 +6565,6 @@ build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,
     {
       tree min = build_min_non_dep (COND_EXPR, expr,
 				    orig_ifexp, orig_op1, orig_op2);
-      /* Remember that the result is an lvalue or xvalue.  */
-      if (glvalue_p (expr) && !glvalue_p (min))
-	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),
-						   !lvalue_p (expr));
       expr = convert_from_reference (min);
     }
   return expr;
diff --git a/gcc/testsuite/g++.dg/pr84231.C b/gcc/testsuite/g++.dg/pr84231.C
new file mode 100644
index 000000000000..de7c89a2ab69
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr84231.C
@@ -0,0 +1,29 @@
+// PR c++/84231 - overload resolution with cond_expr in a template
+
+// { dg-do compile }
+
+struct format {
+  template<typename T> format& operator%(const T&) { return *this; }
+  template<typename T> format& operator%(T&) { return *this; }
+};
+
+format f;
+
+template <typename>
+void function_template(bool b)
+{
+  // Compiles OK with array lvalue:
+  f % (b ? "x" : "x");
+
+  // Used to fails with pointer rvalue:
+  f % (b ? "" : "x");
+}
+
+void normal_function(bool b)
+{
+  // Both cases compile OK in non-template function:
+  f % (b ? "x" : "x");
+  f % (b ? "" : "x");
+
+  function_template<void>(b);
+}


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 2, 2018, 5:14 p.m. | #7
On Fri, Mar 2, 2018 at 2:57 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 28, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Wed, Feb 28, 2018 at 12:24 AM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> +  if (processing_template_decl)

>>> +    result_type = cp_build_reference_type (result_type, !is_lvalue);

>

>> If !is_lvalue, we don't want a reference type at all, as the result is

>> a prvalue.  is_lvalue should probably rename to is_glvalue.

>

> I ended up moving the above to the path in which we deal with lvalues

> and xvalues.  I still renamed the variable as suggested, though I no

> longer use it.

>

>> The second argument to cp_build_reference_type should be xvalue_p (arg2).

>

> I thought of adding a comment as to why testing just arg2 was correct,

> but then moving the code kind of made it obvious, didn't it?

>

>>> +      /* Except for type-dependent exprs, a REFERENCE_TYPE will

>>> +        indicate whether its result is an lvalue or so.

>

>> "or not" ?

>

> I meant "or so" as in "or other kinds of reference values".

>

>>> +      if (processing_template_decl

>>> +         && !type_dependent_expression_p (CONST_CAST_TREE (ref)))

>>> +       return clk_none;

>

>> I think we want to return clk_class for a COND_EXPR of class type.

>

> or array, like the default case, I suppose.

>

>> Can we actually get here for a type-dependent expression?  I'd think

>> we'd never get as far as asking whether a type-dependent expression is

>> an lvalue, since in general we can't answer that question.

>

> We shouldn't, indeed.  And AFAICT we don't; I've added an assert to make

> sure.

>

> [C++] [PR84231] overload on cond_expr in template

>

> A non-type-dependent COND_EXPR within a template is reconstructed with

> the original operands, after one with non-dependent proxies is built to

> determine its result type.  This is problematic because the operands of

> a COND_EXPR determined to be an rvalue may have been converted to denote

> their rvalue nature.  The reconstructed one, however, won't have such

> conversions, so lvalue_kind may not recognize it as an rvalue, which may

> lead to e.g. incorrect overload resolution decisions.

>

> If we mistake such a COND_EXPR for an lvalue, overload resolution might

> regard a conversion sequence that binds it to a non-const reference as

> viable, and then select that over one that binds it to a const

> reference.  Only after template substitution would we rebuild the

> COND_EXPR, realize it is an rvalue, and conclude the reference binding

> is ill-formed, but at that point we'd have long discarded any alternate

> candidates we could have used.

>

> This patch modifies the logic that determines whether a

> (non-type-dependent) COND_EXPR in a template is an lvalue, to rely on

> its type, more specifically, on the presence of a REFERENCE_TYPE

> wrapper.  In order to avoid a type bootstrapping problem, the

> REFERENCE_TYPE that wraps the type of some such COND_EXPRs is

> introduced earlier, so that we don't have to test for lvalueness of

> the expression using the very code that we wish to change.

>

>

> for  gcc/cp/ChangeLog

>

>         PR c++/84231

>         * tree.c (lvalue_kind): Use presence/absence of REFERENCE_TYPE

>         only while processing template decls.

>         * typeck.c (build_x_conditional_expr): Move wrapping of

>         reference type around type...

>         * call.c (build_conditional_expr_1): ... here.  Rename

>         is_lvalue to is_glvalue.

>         * parser.c (cp_parser_fold_expression): Catch REFERENCE_REF_P

>         INDIRECT_REF of COND_EXPR too.

>

> for  gcc/testsuite/ChangeLog

>

>         PR c++/84231

>         * g++.dg/pr84231.C: New.

> ---

>  gcc/cp/call.c                  |   11 +++++++----

>  gcc/cp/parser.c                |    4 +++-

>  gcc/cp/tree.c                  |   15 +++++++++++++++

>  gcc/cp/typeck.c                |    4 ----

>  gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++

>  5 files changed, 54 insertions(+), 9 deletions(-)

>  create mode 100644 gcc/testsuite/g++.dg/pr84231.C

>

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> index 11fe28292fb1..6707aa2d3f02 100644

> --- a/gcc/cp/call.c

> +++ b/gcc/cp/call.c

> @@ -4782,7 +4782,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,

>    tree arg3_type;

>    tree result = NULL_TREE;

>    tree result_type = NULL_TREE;

> -  bool is_lvalue = true;

> +  bool is_glvalue = true;

>    struct z_candidate *candidates = 0;

>    struct z_candidate *cand;

>    void *p;

> @@ -5037,7 +5037,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,

>           return error_mark_node;

>         }

>

> -      is_lvalue = false;

> +      is_glvalue = false;

>        goto valid_operands;

>      }

>    /* [expr.cond]

> @@ -5155,6 +5155,9 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,

>        && same_type_p (arg2_type, arg3_type))

>      {

>        result_type = arg2_type;

> +      if (processing_template_decl)

> +       result_type = cp_build_reference_type (result_type, xvalue_p (arg2));


Let's add a comment along the lines of

/* Let lvalue_kind know this was a glvalue.  */

OK with that change.

Jason
Alexandre Oliva March 6, 2018, 6:13 a.m. | #8
On Mar  2, 2018, Jason Merrill <jason@redhat.com> wrote:

> Let's add a comment along the lines of


> /* Let lvalue_kind know this was a glvalue.  */


> OK with that change.


Thanks, here's what I'm about to check in.

[C++] [PR84231] overload on cond_expr in template

A non-type-dependent COND_EXPR within a template is reconstructed with
the original operands, after one with non-dependent proxies is built to
determine its result type.  This is problematic because the operands of
a COND_EXPR determined to be an rvalue may have been converted to denote
their rvalue nature.  The reconstructed one, however, won't have such
conversions, so lvalue_kind may not recognize it as an rvalue, which may
lead to e.g. incorrect overload resolution decisions.

If we mistake such a COND_EXPR for an lvalue, overload resolution might
regard a conversion sequence that binds it to a non-const reference as
viable, and then select that over one that binds it to a const
reference.  Only after template substitution would we rebuild the
COND_EXPR, realize it is an rvalue, and conclude the reference binding
is ill-formed, but at that point we'd have long discarded any alternate
candidates we could have used.

This patch modifies the logic that determines whether a
(non-type-dependent) COND_EXPR in a template is an lvalue, to rely on
its type, more specifically, on the presence of a REFERENCE_TYPE
wrapper.  In order to avoid a type bootstrapping problem, the
REFERENCE_TYPE that wraps the type of some such COND_EXPRs is
introduced earlier, so that we don't have to test for lvalueness of
the expression using the very code that we wish to change.


for  gcc/cp/ChangeLog

	PR c++/84231
	* tree.c (lvalue_kind): Use presence/absence of REFERENCE_TYPE
	only while processing template decls.
	* typeck.c (build_x_conditional_expr): Move wrapping of
	reference type around type...
	* call.c (build_conditional_expr_1): ... here.  Rename
	is_lvalue to is_glvalue.
	* parser.c (cp_parser_fold_expression): Catch REFERENCE_REF_P
	INDIRECT_REF of COND_EXPR too.

for  gcc/testsuite/ChangeLog

	PR c++/84231
	* g++.dg/pr84231.C: New.
---
 gcc/cp/call.c                  |   12 ++++++++----
 gcc/cp/parser.c                |    4 +++-
 gcc/cp/tree.c                  |   15 +++++++++++++++
 gcc/cp/typeck.c                |    4 ----
 gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++
 5 files changed, 55 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr84231.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 11fe28292fb1..f83d51f3457e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -4782,7 +4782,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
   tree arg3_type;
   tree result = NULL_TREE;
   tree result_type = NULL_TREE;
-  bool is_lvalue = true;
+  bool is_glvalue = true;
   struct z_candidate *candidates = 0;
   struct z_candidate *cand;
   void *p;
@@ -5037,7 +5037,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
 	  return error_mark_node;
 	}
 
-      is_lvalue = false;
+      is_glvalue = false;
       goto valid_operands;
     }
   /* [expr.cond]
@@ -5155,6 +5155,10 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
       && same_type_p (arg2_type, arg3_type))
     {
       result_type = arg2_type;
+      if (processing_template_decl)
+	/* Let lvalue_kind know this was a glvalue.  */
+	result_type = cp_build_reference_type (result_type, xvalue_p (arg2));
+
       arg2 = mark_lvalue_use (arg2);
       arg3 = mark_lvalue_use (arg3);
       goto valid_operands;
@@ -5167,7 +5171,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
      cv-qualified) class type, overload resolution is used to
      determine the conversions (if any) to be applied to the operands
      (_over.match.oper_, _over.built_).  */
-  is_lvalue = false;
+  is_glvalue = false;
   if (!same_type_p (arg2_type, arg3_type)
       && (CLASS_TYPE_P (arg2_type) || CLASS_TYPE_P (arg3_type)))
     {
@@ -5361,7 +5365,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
   /* We can't use result_type below, as fold might have returned a
      throw_expr.  */
 
-  if (!is_lvalue)
+  if (!is_glvalue)
     {
       /* Expand both sides into the same slot, hopefully the target of
 	 the ?: expression.  We used to check for TARGET_EXPRs here,
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index e1acb07d29ef..f21257f41e7b 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -4963,7 +4963,9 @@ cp_parser_fold_expression (cp_parser *parser, tree expr1)
   else if (is_binary_op (TREE_CODE (expr1)))
     error_at (location_of (expr1),
 	      "binary expression in operand of fold-expression");
-  else if (TREE_CODE (expr1) == COND_EXPR)
+  else if (TREE_CODE (expr1) == COND_EXPR
+	   || (REFERENCE_REF_P (expr1)
+	       && TREE_CODE (TREE_OPERAND (expr1, 0)) == COND_EXPR))
     error_at (location_of (expr1),
 	      "conditional expression in operand of fold-expression");
 
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 19f1c0629c9a..4cf2126608f0 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -194,6 +194,21 @@ lvalue_kind (const_tree ref)
       break;
 
     case COND_EXPR:
+      if (processing_template_decl)
+	{
+	  /* Within templates, a REFERENCE_TYPE will indicate whether
+	     the COND_EXPR result is an ordinary lvalue or rvalueref.
+	     Since REFERENCE_TYPEs are handled above, if we reach this
+	     point, we know we got a plain rvalue.  Unless we have a
+	     type-dependent expr, that is, but we shouldn't be testing
+	     lvalueness if we can't even tell the types yet!  */
+	  gcc_assert (!type_dependent_expression_p (CONST_CAST_TREE (ref)));
+	  if (CLASS_TYPE_P (TREE_TYPE (ref))
+	      || TREE_CODE (TREE_TYPE (ref)) == ARRAY_TYPE)
+	    return clk_class;
+	  else
+	    return clk_none;
+	}
       op1_lvalue_kind = lvalue_kind (TREE_OPERAND (ref, 1)
 				    ? TREE_OPERAND (ref, 1)
 				    : TREE_OPERAND (ref, 0));
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 0e7c63dd1973..fba04c49ec2d 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -6565,10 +6565,6 @@ build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,
     {
       tree min = build_min_non_dep (COND_EXPR, expr,
 				    orig_ifexp, orig_op1, orig_op2);
-      /* Remember that the result is an lvalue or xvalue.  */
-      if (glvalue_p (expr) && !glvalue_p (min))
-	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),
-						   !lvalue_p (expr));
       expr = convert_from_reference (min);
     }
   return expr;
diff --git a/gcc/testsuite/g++.dg/pr84231.C b/gcc/testsuite/g++.dg/pr84231.C
new file mode 100644
index 000000000000..de7c89a2ab69
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr84231.C
@@ -0,0 +1,29 @@
+// PR c++/84231 - overload resolution with cond_expr in a template
+
+// { dg-do compile }
+
+struct format {
+  template<typename T> format& operator%(const T&) { return *this; }
+  template<typename T> format& operator%(T&) { return *this; }
+};
+
+format f;
+
+template <typename>
+void function_template(bool b)
+{
+  // Compiles OK with array lvalue:
+  f % (b ? "x" : "x");
+
+  // Used to fails with pointer rvalue:
+  f % (b ? "" : "x");
+}
+
+void normal_function(bool b)
+{
+  // Both cases compile OK in non-template function:
+  f % (b ? "x" : "x");
+  f % (b ? "" : "x");
+
+  function_template<void>(b);
+}


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Patch

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 83e767829986..25ac44e57772 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -6560,12 +6560,26 @@  build_x_conditional_expr (location_t loc, tree ifexp, tree op1, tree op2,
   expr = build_conditional_expr (loc, ifexp, op1, op2, complain);
   if (processing_template_decl && expr != error_mark_node)
     {
+      bool rval = !glvalue_p (expr);
       tree min = build_min_non_dep (COND_EXPR, expr,
 				    orig_ifexp, orig_op1, orig_op2);
+      bool mrval = !glvalue_p (min);
       /* Remember that the result is an lvalue or xvalue.  */
-      if (glvalue_p (expr) && !glvalue_p (min))
+      if (!rval && mrval)
 	TREE_TYPE (min) = cp_build_reference_type (TREE_TYPE (min),
 						   !lvalue_p (expr));
+      else if (rval && !mrval)
+	{
+	  /* If it was supposed to be an rvalue but it's not, adjust
+	     one of the operands so that any overload resolution
+	     taking this COND_EXPR as an operand makes the correct
+	     decisions.  See c++/84231.  */
+	  TREE_OPERAND (min, 2) = build1_loc (loc, NON_LVALUE_EXPR,
+					      TREE_TYPE (min),
+					      TREE_OPERAND (min, 2));
+	  EXPR_LOCATION_WRAPPER_P (TREE_OPERAND (min, 2)) = 1;
+	  gcc_checking_assert (!glvalue_p (min));
+	}
       expr = convert_from_reference (min);
     }
   return expr;
diff --git a/gcc/testsuite/g++.dg/pr84231.C b/gcc/testsuite/g++.dg/pr84231.C
new file mode 100644
index 000000000000..de7c89a2ab69
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr84231.C
@@ -0,0 +1,29 @@ 
+// PR c++/84231 - overload resolution with cond_expr in a template
+
+// { dg-do compile }
+
+struct format {
+  template<typename T> format& operator%(const T&) { return *this; }
+  template<typename T> format& operator%(T&) { return *this; }
+};
+
+format f;
+
+template <typename>
+void function_template(bool b)
+{
+  // Compiles OK with array lvalue:
+  f % (b ? "x" : "x");
+
+  // Used to fails with pointer rvalue:
+  f % (b ? "" : "x");
+}
+
+void normal_function(bool b)
+{
+  // Both cases compile OK in non-template function:
+  f % (b ? "x" : "x");
+  f % (b ? "" : "x");
+
+  function_template<void>(b);
+}