From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id 2E3233858D37 for ; Mon, 13 Jul 2020 20:48:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2E3233858D37 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-139-sMdDWLoMNr-4sppk1i2_qg-1; Mon, 13 Jul 2020 16:48:14 -0400 X-MC-Unique: sMdDWLoMNr-4sppk1i2_qg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 21A058015CE for ; Mon, 13 Jul 2020 20:48:13 +0000 (UTC) Received: from redhat.com (ovpn-119-223.rdu2.redhat.com [10.10.119.223]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9A8F260BEC; Mon, 13 Jul 2020 20:48:12 +0000 (UTC) Date: Mon, 13 Jul 2020 16:48:10 -0400 From: Marek Polacek To: Jason Merrill Cc: GCC Patches Subject: Re: [PATCH v3] c++: Make convert_like complain about bad ck_ref_bind again [PR95789] Message-ID: <20200713204810.GA103559@redhat.com> References: <20200623020927.172479-1-polacek@redhat.com> <44caf579-8fcc-34b3-0aa0-0468e159a747@redhat.com> <20200708203522.GO103559@redhat.com> <49d626e0-f3c9-73cf-5b6f-fe9547b567ca@redhat.com> MIME-Version: 1.0 In-Reply-To: <49d626e0-f3c9-73cf-5b6f-fe9547b567ca@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-15.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jul 2020 20:48:18 -0000 On Sat, Jul 11, 2020 at 10:32:55AM -0400, Jason Merrill via Gcc-patches wrote: > 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? Perhaps like this? convert_like_real_1 is unsightly and I have plans to fix that (and get rid of the convert_like macro), but that's a clean-up, not a bug fix so it's not included in this patch. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? -- >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. This I achieved by adding a wrapper function. gcc/cp/ChangeLog: PR c++/95789 PR c++/96104 PR c++/96179 * call.c (convert_like_real_1): Renamed from convert_like_real. (convert_like_real): New wrapper for convert_like_real_1. gcc/testsuite/ChangeLog: PR c++/95789 PR c++/96104 PR c++/96179 * g++.dg/conversion/ref4.C: New test. * g++.dg/conversion/ref5.C: New test. * g++.dg/conversion/ref6.C: New test. --- gcc/cp/call.c | 54 ++++++++++++++++++-------- gcc/testsuite/g++.dg/conversion/ref4.C | 22 +++++++++++ gcc/testsuite/g++.dg/conversion/ref5.C | 14 +++++++ gcc/testsuite/g++.dg/conversion/ref6.C | 24 ++++++++++++ 4 files changed, 98 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/conversion/ref4.C create mode 100644 gcc/testsuite/g++.dg/conversion/ref5.C create mode 100644 gcc/testsuite/g++.dg/conversion/ref6.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 5341a572980..6d5d5e801a5 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -171,6 +171,8 @@ static tree build_over_call (struct z_candidate *, int, tsubst_flags_t); /*c_cast_p=*/false, (COMPLAIN)) static tree convert_like_real (conversion *, tree, tree, int, bool, bool, tsubst_flags_t); +static tree convert_like_real_1 (conversion *, tree, tree, int, bool, + bool, tsubst_flags_t); static void op_error (const op_location_t &, enum tree_code, enum tree_code, tree, tree, tree, bool); static struct z_candidate *build_user_type_conversion_1 (tree, tree, int, @@ -7281,6 +7283,39 @@ maybe_warn_array_conv (location_t loc, conversion *c, tree expr) "are only available with %<-std=c++20%> or %<-std=gnu++20%>"); } +/* Wrapper for convert_like_real_1 that handles creating IMPLICIT_CONV_EXPR. */ + +static tree +convert_like_real (conversion *convs, tree expr, tree fn, int argnum, + bool issue_conversion_warnings, + bool c_cast_p, tsubst_flags_t complain) +{ + /* Creating &TARGET_EXPR<> in a template breaks when substituting, + and creating a CALL_EXPR in a template breaks in finish_call_expr + 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 (convs->type) || CLASS_TYPE_P (TREE_TYPE (expr)))) + { + conv_expr = build1 (IMPLICIT_CONV_EXPR, convs->type, 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. */ + } + expr = convert_like_real_1 (convs, expr, fn, argnum, + issue_conversion_warnings, c_cast_p, complain); + if (expr == error_mark_node) + return error_mark_node; + return conv_expr ? conv_expr : expr; +} + /* Perform the conversions in CONVS on the expression EXPR. FN and ARGNUM are used for diagnostics. ARGNUM is zero based, -1 indicates the `this' argument of a method. INNER is nonzero when @@ -7292,9 +7327,9 @@ maybe_warn_array_conv (location_t loc, conversion *c, tree expr) conversions to inaccessible bases are permitted. */ static tree -convert_like_real (conversion *convs, tree expr, tree fn, int argnum, - bool issue_conversion_warnings, - bool c_cast_p, tsubst_flags_t complain) +convert_like_real_1 (conversion *convs, tree expr, tree fn, int argnum, + bool issue_conversion_warnings, + bool c_cast_p, tsubst_flags_t complain) { tree totype = convs->type; diagnostic_t diag_kind; @@ -7395,19 +7430,6 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum, if (issue_conversion_warnings && (complain & tf_warning)) conversion_null_warnings (totype, expr, fn, argnum); - /* Creating &TARGET_EXPR<> in a template breaks when substituting, - and creating a CALL_EXPR in a template breaks in finish_call_expr - so use an IMPLICIT_CONV_EXPR for this conversion. We would have - created such codes e.g. when calling a user-defined conversion - function. */ - 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); - } - switch (convs->kind) { case ck_user: 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 +struct A { + B& get() const { return f; } // { dg-error "binding reference" } + + B f; +}; + +int main() { + A 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 void fn(T &); +class E {}; +struct F { + template void mfn(T t) { t, fn(E()); } // { dg-error "cannot bind non-const lvalue reference" } +}; +int +main() +{ + E e; + F f; + f.mfn(e); +} diff --git a/gcc/testsuite/g++.dg/conversion/ref6.C b/gcc/testsuite/g++.dg/conversion/ref6.C new file mode 100644 index 00000000000..fc87199053c --- /dev/null +++ b/gcc/testsuite/g++.dg/conversion/ref6.C @@ -0,0 +1,24 @@ +// PR c++/96179 +// { dg-do compile { target c++11 } } + +template struct vector +{ + void push_back(T) { } +}; + +struct dummy{ + int a; +}; + +void Modify_Dummy(dummy &d){ + d.a=1; +} + +template void Templated_Function(){ + vector A; + A.push_back(Modify_Dummy(dummy{0})); // { dg-error "cannot bind non-const lvalue reference" } +} + +int main(){ + Templated_Function(); +} base-commit: 9cba898481368ce16c6a2d30ef781a82dce27c55 -- 2.26.2