From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24402 invoked by alias); 3 Apr 2018 17:33:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 24391 invoked by uid 89); 3 Apr 2018 17:33:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=contextual, gotten X-HELO: mail-ot0-f182.google.com Received: from mail-ot0-f182.google.com (HELO mail-ot0-f182.google.com) (74.125.82.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Apr 2018 17:33:08 +0000 Received: by mail-ot0-f182.google.com with SMTP id a14-v6so2129553otf.6 for ; Tue, 03 Apr 2018 10:33:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UBudSdKGpnisn1dB45F/KfktFHTvUejSBbdg0oFqPqc=; b=GCP4MXhx4Hz6wAnxo2yCkvdDE+3IqjpKKNiJ+li5ClT9v8sLZ/CDIZ3dVmjF7Ubtax 4cIM/9Es3+uNoEYSjXsPOOiBjLGPDjVkQgIKyvOJ9oJv2+72bzoaXXw4NlNayLTVI7Jl y+t4y1kq0n5JM/k25wVVnrvimBucP9lImtWy0zB92AbtEZ1/QOxMYqQZS8s0clsRWB1N pdOQYUzWGZMeAegBB7C8RV9iLmrADZIVyeNd1s0r/iUI/K8siAOyFXLppSHDwHXRv7W/ w7v2va6Ek+6dtq1KcX44LuG5T5uLg9Bbv2vc7yVlGuhfFV84EAr15IHr9p19Uo25NEBb OZ6Q== X-Gm-Message-State: ALQs6tAdyVzVSX1gluDlsFzJo5XyMiIkhmUNlPgH+VlCY14de83HO3jc xv/mzgASknelyB2Tqr8XmXkS/nGlPVIFAK4XveUuzQ== X-Google-Smtp-Source: AIpwx4/NBELgeR9vvXOU+IZtmKgyOH2top7sPd55po/23zTorTKjDoO1jwvIk9QW+7le070QfczW7wNaj4cgnUVBjYQ= X-Received: by 2002:a9d:29ae:: with SMTP id n43-v6mr9288349otb.313.1522776786790; Tue, 03 Apr 2018 10:33:06 -0700 (PDT) MIME-Version: 1.0 Received: by 10.201.7.234 with HTTP; Tue, 3 Apr 2018 10:32:46 -0700 (PDT) In-Reply-To: <20180321201430.GH3522@redhat.com> References: <20180314155956.GU3522@redhat.com> <20180315114922.GV3522@redhat.com> <20180321183739.GG3522@redhat.com> <20180321201430.GH3522@redhat.com> From: Jason Merrill Date: Tue, 03 Apr 2018 17:33:00 -0000 Message-ID: Subject: Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854) To: Marek Polacek Cc: GCC Patches Content-Type: multipart/mixed; boundary="0000000000007398870568f51995" X-IsSubscribed: yes X-SW-Source: 2018-04/txt/msg00108.txt.bz2 --0000000000007398870568f51995 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-length: 5615 On Wed, Mar 21, 2018 at 4:14 PM, Marek Polacek wrote: > On Wed, Mar 21, 2018 at 02:55:36PM -0400, Jason Merrill wrote: >> On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek wrot= e: >> > On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote: >> >> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek w= rote: >> >> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote: >> >> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek wrote: >> >> >> > cxx_constant_value doesn't understand template codes, and neithe= r it >> >> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash= . Here >> >> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it cal= ls >> >> >> > is_nondependent_constant_expression which checks type_unknown_p,= it left the >> >> >> > expression as it was. We can't use is_nondependent_constant_exp= ression in >> >> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expressio= n and that is >> >> >> > not suitable here; we'd miss diagnostics. So I did the followin= g; I think we >> >> >> > should reject the testcase with an error. >> >> >> > >> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> >> > >> >> >> > 2018-03-14 Marek Polacek >> >> >> > >> >> >> > PR c++/84854 >> >> >> > * semantics.c (finish_if_stmt_cond): Give error if the c= ondition >> >> >> > is an overloaded function with no contextual type inform= ation. >> >> >> > >> >> >> > * g++.dg/cpp1z/constexpr-if15.C: New test. >> >> >> > >> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c >> >> >> > index fdf37bea770..a056e9445e9 100644 >> >> >> > --- gcc/cp/semantics.c >> >> >> > +++ gcc/cp/semantics.c >> >> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stm= t) >> >> >> > && require_constant_expression (cond) >> >> >> > && !value_dependent_expression_p (cond)) >> >> >> > { >> >> >> > - cond =3D instantiate_non_dependent_expr (cond); >> >> >> > - cond =3D cxx_constant_value (cond, NULL_TREE); >> >> >> > + if (type_unknown_p (cond)) >> >> >> > + { >> >> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond)); >> >> >> > + cond =3D error_mark_node; >> >> >> >> >> >> I think I'd prefer to skip this block when type_unknown_p, and lea= ve >> >> >> error handling up to the code shared with regular if. >> >> > >> >> > Like this? >> >> >> >> Yes, thanks. >> >> >> >> > It was my first version, but I thought we wanted the error; >> >> >> >> Getting an error is an improvement, but it should apply to >> >> non-constexpr if as well, so checking in maybe_convert_cond would be >> >> better. Actually, if we deal with unknown type there, we shouldn't >> >> need this patch at all. >> >> >> >> ...except I also notice that since maybe_convert_cond doesn't do any >> >> conversion in a template, we're trying to extract the constant value >> >> without first converting to bool, which breaks this testcase: >> >> >> >> struct A >> >> { >> >> constexpr operator bool () { return true; } >> >> int i; >> >> }; >> >> >> >> A a; >> >> >> >> template void f() >> >> { >> >> constexpr bool b =3D a; // works >> >> if constexpr (a) { } // breaks >> >> } >> >> >> >> int main() >> >> { >> >> f(); >> >> } >> >> >> >> A simple fix would be to replace your type_unknown_p check here with a >> >> comparison to boolean_type_node, so we wait until instantiation time >> >> to require a constant value. >> > >> > Ok, that works. >> > >> > We should also make g++ accept the testcase with "static_assert(a)" in= stead of >> > "if constexpr (a) { }" probably. >> >> > I guess the cxx_constant_value call in >> > finish_static_assert should happen earlier? >> >> fold_non_dependent_expr should already have gotten the constant value, >> the call to cxx_constant_value is just for giving an error. > > Oop, right. > >> The bug seems to be that is_nondependent_constant_expression doesn't >> realize that the conversion to bool is OK because it uses a constexpr >> member function. > > OK, I can look into this separately. > >> >> Better would be to actually do the conversion. Perhaps this could >> >> share code (for converting and getting a constant value) with >> >> finish_static_assert. >> > >> > But this I didn't manage to make to work. If I call perform_implicit_= conversion_flags >> > in maybe_convert_cond, I get >> > error: cannot resolve overloaded function =E2=80=98foo=E2=80=99 based = on conversion to type =E2=80=98bool=E2=80=99 >> > so I'm not sure how the conversion would help. >> >> That looks like a good diagnostic to me, what's the problem? > > Ugh, I got this wrong. That diagnostic is fine, because we can reject > constexpr-if15.C, but with perform_implicit_conversion_flags we then > can't evaluate constexpr-if16.C: > error: the value of =E2=80=98a=E2=80=99 is not usable in a constant expre= ssion > >> > Anyway, here's at least the boolean_type_node version. >> > >> > Bootstrapped/regtested on x86_64-linux. >> > >> > 2018-03-21 Marek Polacek >> > >> > PR c++/84854 >> > * semantics.c (finish_if_stmt_cond): Check if the type of the = condition >> > is boolean. >> >> OK. > > Thanks, will commit this part along with the testcases. Incrementally we= should > then a) add constexpr-if15.C diagnostic, b) accept constexpr-if16.C with > static_assert, too. I think we want to use instantiation_dependent_expression_p here, too. Applying. --0000000000007398870568f51995 Content-Type: text/x-patch; charset="US-ASCII"; name="if-inst-dep.diff" Content-Disposition: attachment; filename="if-inst-dep.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_jfjxx42o0 Content-length: 1078 Y29tbWl0IDI1NWQzM2Y3ZTEwYmQxM2U5OWIyNzBlN2FjMzQ5NDZlM2EwM2Ri YTEKQXV0aG9yOiBKYXNvbiBNZXJyaWxsIDxqYXNvbkByZWRoYXQuY29tPgpE YXRlOiAgIE1vbiBBcHIgMiAxNzo1ODo0MSAyMDE4IC0wNDAwCgogICAgKiBz ZW1hbnRpY3MuYyAoZmluaXNoX2lmX3N0bXRfY29uZCk6IFVzZSBpbnN0YW50 aWF0aW9uX2RlcGVuZGVudF9leHByZXNzaW9uX3AuCgpkaWZmIC0tZ2l0IGEv Z2NjL2NwL3NlbWFudGljcy5jIGIvZ2NjL2NwL3NlbWFudGljcy5jCmluZGV4 IGVlZjllMmY2NDVkLi5lZjI0M2Y2YmYwYSAxMDA2NDQKLS0tIGEvZ2NjL2Nw L3NlbWFudGljcy5jCisrKyBiL2djYy9jcC9zZW1hbnRpY3MuYwpAQCAtNzMz LDcgKzczMyw3IEBAIGZpbmlzaF9pZl9zdG10X2NvbmQgKHRyZWUgY29uZCwg dHJlZSBpZl9zdG10KQogICBpZiAoSUZfU1RNVF9DT05TVEVYUFJfUCAoaWZf c3RtdCkKICAgICAgICYmICF0eXBlX2RlcGVuZGVudF9leHByZXNzaW9uX3Ag KGNvbmQpCiAgICAgICAmJiByZXF1aXJlX2NvbnN0YW50X2V4cHJlc3Npb24g KGNvbmQpCi0gICAgICAmJiAhdmFsdWVfZGVwZW5kZW50X2V4cHJlc3Npb25f cCAoY29uZCkKKyAgICAgICYmICFpbnN0YW50aWF0aW9uX2RlcGVuZGVudF9l eHByZXNzaW9uX3AgKGNvbmQpCiAgICAgICAvKiBXYWl0IHVudGlsIGluc3Rh bnRpYXRpb24gdGltZSwgc2luY2Ugb25seSB0aGVuIENPTkQgaGFzIGJlZW4K IAkgY29udmVydGVkIHRvIGJvb2wuICAqLwogICAgICAgJiYgVFJFRV9UWVBF IChjb25kKSA9PSBib29sZWFuX3R5cGVfbm9kZSkK --0000000000007398870568f51995--