| Message ID | orvaf6gbhl.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [C++,PR84231] overload on cond_expr in template | expand |
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
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,
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
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); +}
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
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); +}
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
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); +}
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); +}