On 7/8/20 4:35 PM, Marek Polacek wrote:
On Fri, Jul 03, 2020 at 05:24:34PM -0400, Jason Merrill via Gcc-patches wrote:
On 6/22/20 10:09 PM, Marek Polacek wrote:
convert_like issues errors about bad_p conversions at the beginning
of the function, but in the ck_ref_bind case, it only issues them
after we've called convert_like on the next conversion.

This doesn't work as expected since r10-7096 because when we see
a conversion from/to class type in a template, we return early, thereby
missing the error, and a bad_p conversion goes by undetected.  That
made the attached test to compile even though it should not.

Hmm, why isn't there an error at instantiation time?

We threw away the result: we're called from

12213       if (complain & tf_error)
12214         {
12215           if (conv)
12216             convert_like (conv, expr, complain);
...
12228       return error_mark_node;

and convert_like never saw converting this->f to B& again when instantiating.

Though giving an error at template parsing time is definitely preferable.

Yup.

I had thought that I could just move the ck_ref_bind/bad_p errors
above to the rest of them, but that regressed diagnostics because
expr then wasn't converted yet by the nested convert_like_real call.

Yeah, the early section is really just for scalar conversions.

It would probably be good to do normal processing for all other bad
conversions and only afterward build the IMPLICIT_CONV_EXPR if we aren't
returning error_mark_node.

Ok, so that if we add more bad_p errors, we won't run into this again.

Unfortunately it's a bit ugly.  I could introduce a RETURN macro to
use RETURN (expr); instead of what I have now, but it wouldn't be simply
"conv_expr ? conv_expr : expr", because if we have error_mark_node, we
want to return that, not conv_expr.  Does that seem worth it?
(I wish I could at least use the op0 ?: op1 GNU extension.)

Or introduce a wrapper function that handles IMPLICIT_CONV_EXPR?

I've also added another test.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --
convert_like issues errors about bad_p conversions at the beginning
of the function, but in the ck_ref_bind case, it only issues them
after we've called convert_like on the next conversion.

This doesn't work as expected since r10-7096 because when we see
a conversion from/to class type in a template, we return early, thereby
missing the error, and a bad_p conversion goes by undetected.  That
made the attached test to compile even though it should not.

I had thought that I could just move the ck_ref_bind/bad_p errors
above to the rest of them, but that regressed diagnostics because
expr then wasn't converted yet by the nested convert_like_real call.

So, for bad_p conversions, do the normal processing, but still return
the IMPLICIT_CONV_EXPR to avoid introducing trees that the template
processing can't handle well.

gcc/cp/ChangeLog:

        PR c++/95789
        * call.c (convert_like_real): Do the normal processing for
        conversion that are bad_p.  Return the IMPLICIT_CONV_EXPR
        instead of EXPR if we're processing a bad_p conversion in
        a template.

gcc/testsuite/ChangeLog:

        PR c++/95789
        * g++.dg/conversion/ref4.C: New test.
        * g++.dg/conversion/ref5.C: New test.
---
  gcc/cp/call.c                          | 47 +++++++++++++++-----------
  gcc/testsuite/g++.dg/conversion/ref4.C | 22 ++++++++++++
  gcc/testsuite/g++.dg/conversion/ref5.C | 14 ++++++++
  3 files changed, 64 insertions(+), 19 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/conversion/ref4.C
  create mode 100644 gcc/testsuite/g++.dg/conversion/ref5.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 5341a572980..65565fc90a8 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7400,12 +7400,19 @@ convert_like_real (conversion *convs, tree expr, tree 
fn, int argnum,
       so use an IMPLICIT_CONV_EXPR for this conversion.  We would have
       created such codes e.g. when calling a user-defined conversion
       function.  */
+  tree conv_expr = NULL_TREE;
    if (processing_template_decl
        && convs->kind != ck_identity
        && (CLASS_TYPE_P (totype) || CLASS_TYPE_P (TREE_TYPE (expr))))
      {
-      expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
-      return convs->kind == ck_ref_bind ? expr : convert_from_reference (expr);
+      conv_expr = build1 (IMPLICIT_CONV_EXPR, totype, expr);
+      if (convs->kind != ck_ref_bind)
+       conv_expr = convert_from_reference (conv_expr);
+      if (!convs->bad_p)
+       return conv_expr;
+      /* Do the normal processing to give the bad_p errors.  But we still
+        need to return the IMPLICIT_CONV_EXPR, unless we're returning
+        error_mark_node.  */
      }
switch (convs->kind)
@@ -7465,7 +7472,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
                TARGET_EXPR_LIST_INIT_P (expr) = true;
                TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
              }
-           return expr;
+           return conv_expr ? conv_expr : expr;
          }
/* We don't know here whether EXPR is being used as an lvalue or
@@ -7488,7 +7495,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
              TARGET_EXPR_LIST_INIT_P (expr) = true;
          }
- return expr;
+       return conv_expr ? conv_expr : expr;
        }
      case ck_identity:
        if (BRACE_ENCLOSED_INITIALIZER_P (expr))
@@ -7513,7 +7520,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
           continue to warn about uses of EXPR as an integer, rather than as a
           pointer.  */
        expr = build_int_cst (totype, 0);
-      return expr;
+      return conv_expr ? conv_expr : expr;
      case ck_ambig:
        /* We leave bad_p off ck_ambig because overload resolution considers
         it valid, it just fails when we try to perform it.  So we need to
@@ -7585,7 +7592,8 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
        field = next_initializable_field (DECL_CHAIN (field));
        CONSTRUCTOR_APPEND_ELT (vec, field, size_int (len));
        tree new_ctor = build_constructor (totype, vec);
-       return get_target_expr_sfinae (new_ctor, complain);
+       return (conv_expr ? conv_expr
+                         : get_target_expr_sfinae (new_ctor, complain));
        }
case ck_aggr:
@@ -7598,14 +7606,14 @@ convert_like_real (conversion *convs, tree expr, tree 
fn, int argnum,
          imag = perform_implicit_conversion (TREE_TYPE (totype),
                                              imag, complain);
          expr = build2 (COMPLEX_EXPR, totype, real, imag);
-         return expr;
+         return conv_expr ? conv_expr : expr;
        }
        expr = reshape_init (totype, expr, complain);
        expr = get_target_expr_sfinae (digest_init (totype, expr, complain),
                                     complain);
        if (expr != error_mark_node)
        TARGET_EXPR_LIST_INIT_P (expr) = true;
-      return expr;
+      return conv_expr ? conv_expr : expr;
default:
        break;
@@ -7634,19 +7642,19 @@ convert_like_real (conversion *convs, tree expr, tree 
fn, int argnum,
        }
if (! MAYBE_CLASS_TYPE_P (totype))
-       return expr;
+       return conv_expr ? conv_expr : expr;
/* Don't introduce copies when passing arguments along to the inherited
         constructor.  */
        if (current_function_decl
          && flag_new_inheriting_ctors
          && DECL_INHERITED_CTOR (current_function_decl))
-       return expr;
+       return conv_expr ? conv_expr : expr;
if (TREE_CODE (expr) == TARGET_EXPR
          && TARGET_EXPR_LIST_INIT_P (expr))
        /* Copy-list-initialization doesn't actually involve a copy.  */
-       return expr;
+       return conv_expr ? conv_expr : expr;
/* Fall through. */
      case ck_base:
@@ -7657,7 +7665,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
          /* Build an expression for `*((base*) &expr)'.  */
          expr = convert_to_base (expr, totype,
                                  !c_cast_p, /*nonnull=*/true, complain);
-         return expr;
+         return conv_expr ? conv_expr : expr;
        }
/* Copy-initialization where the cv-unqualified version of the source
@@ -7686,7 +7694,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
          maybe_inform_about_fndecl_for_bogus_argument_init (fn, argnum);
        }
- return build_cplus_new (totype, expr, complain);
+      return conv_expr ? conv_expr : build_cplus_new (totype, expr, complain);
case ck_ref_bind:
        {
@@ -7821,16 +7829,16 @@ convert_like_real (conversion *convs, tree expr, tree 
fn, int argnum,
        expr = cp_convert (build_pointer_type (TREE_TYPE (ref_type)),
                           expr, complain);
        /* Convert the pointer to the desired reference type.  */
-       return build_nop (ref_type, expr);
+       return conv_expr ? conv_expr : build_nop (ref_type, expr);
        }
case ck_lvalue:
-      return decay_conversion (expr, complain);
+      return conv_expr ? conv_expr : decay_conversion (expr, complain);
case ck_fnptr:
        /* ??? Should the address of a transaction-safe pointer point to the TM
          clone, and this conversion look up the primary function?  */
-      return build_nop (totype, expr);
+      return conv_expr ? conv_expr : build_nop (totype, expr);
case ck_qual:
        /* Warn about deprecated conversion if appropriate.  */
@@ -7845,11 +7853,12 @@ convert_like_real (conversion *convs, tree expr, tree 
fn, int argnum,
        if (convs->base_p)
        expr = convert_to_base (expr, totype, !c_cast_p,
                                /*nonnull=*/false, complain);
-      return build_nop (totype, expr);
+      return conv_expr ? conv_expr : build_nop (totype, expr);
case ck_pmem:
-      return convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
+      expr = convert_ptrmem (totype, expr, /*allow_inverse_p=*/false,
                             c_cast_p, complain);
+      return conv_expr ? conv_expr : expr;
default:
        break;
@@ -7866,7 +7875,7 @@ convert_like_real (conversion *convs, tree expr, tree fn, 
int argnum,
    else
      expr = cp_convert (totype, expr, complain);
- return expr;
+  return conv_expr ? conv_expr : expr;
  }
/* ARG is being passed to a varargs function. Perform any conversions
diff --git a/gcc/testsuite/g++.dg/conversion/ref4.C 
b/gcc/testsuite/g++.dg/conversion/ref4.C
new file mode 100644
index 00000000000..464a4cf6c0f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref4.C
@@ -0,0 +1,22 @@
+// PR c++/95789
+// { dg-do compile { target c++11 } }
+
+struct B {
+    int n;
+};
+
+template <typename T>
+struct A {
+    B& get() const { return f; } // { dg-error "binding reference" }
+
+    B f;
+};
+
+int main() {
+    A<int> a;
+    a.f = {};
+
+    a.get().n = 10;
+    if (a.f.n != 0)
+      __builtin_abort();
+}
diff --git a/gcc/testsuite/g++.dg/conversion/ref5.C 
b/gcc/testsuite/g++.dg/conversion/ref5.C
new file mode 100644
index 00000000000..0042acd0670
--- /dev/null
+++ b/gcc/testsuite/g++.dg/conversion/ref5.C
@@ -0,0 +1,14 @@
+// PR c++/96104
+
+template <typename T> void fn(T &);
+class E {};
+struct F {
+  template <typename T> void mfn(T t) { t, fn(E()); } // { dg-error "cannot bind 
non-const lvalue reference" }
+};
+int
+main()
+{
+  E e;
+  F f;
+  f.mfn(e);
+}

base-commit: 760df6d296b8fc59796f42dca5eb14012fbfa28b


Reply via email to