From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94957 invoked by alias); 18 Sep 2018 18:29:42 -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 94936 invoked by uid 89); 18 Sep 2018 18:29:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 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= X-HELO: mail-oi0-f67.google.com Received: from mail-oi0-f67.google.com (HELO mail-oi0-f67.google.com) (209.85.218.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Sep 2018 18:29:38 +0000 Received: by mail-oi0-f67.google.com with SMTP id x197-v6so2718049oix.5 for ; Tue, 18 Sep 2018 11:29:38 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:1522:0:0:0:0:0 with HTTP; Tue, 18 Sep 2018 11:29:16 -0700 (PDT) In-Reply-To: <20180918152558.GP16755@redhat.com> References: <20180914171950.GE5587@redhat.com> <20180914204521.GJ5587@redhat.com> <20180917213938.GL5587@redhat.com> <20180918152558.GP16755@redhat.com> From: Jason Merrill Date: Tue, 18 Sep 2018 18:36:00 -0000 Message-ID: Subject: Re: C++ PATCH to implement P1064R0, Virtual Function Calls in Constant Expressions (v4) To: Marek Polacek Cc: GCC Patches , Jakub Jelinek Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg01016.txt.bz2 On Tue, Sep 18, 2018 at 11:25 AM, Marek Polacek wrote: > On Mon, Sep 17, 2018 at 11:28:06PM -0400, Jason Merrill wrote: >> On Mon, Sep 17, 2018 at 5:39 PM, Marek Polacek wrote: >> > On Fri, Sep 14, 2018 at 04:45:22PM -0400, Marek Polacek wrote: >> >> On Fri, Sep 14, 2018 at 04:30:46PM -0400, Jason Merrill wrote: >> >> > On Fri, Sep 14, 2018 at 1:19 PM, Marek Polacek wrote: >> >> > > This patch implements another bit of C++20, virtual calls in constant >> >> > > expression: >> >> > > >> >> > > The basic idea is that since in a constant expression we know the dynamic >> >> > > type (to detect invalid code etc.), the restriction that prohibits virtual >> >> > > calls is unnecessary. >> >> > > >> >> > > Handling virtual function calls turned out to be fairly easy (as anticipated); >> >> > > I simply let the constexpr machinery figure out the dynamic type and then >> >> > > OBJ_TYPE_REF_TOKEN gives us the index into the virtual function table. That >> >> > > way we get the function decl we're interested in, and cxx_eval_call_expression >> >> > > takes it from there. >> >> > > >> >> > > But handling pointer-to-virtual-member-functions doesn't work like that. >> >> > > get_member_function_from_ptrfunc creates a COND_EXPR which looks like >> >> > > if (pf.__pfn & 1) // is it a virtual function? >> >> > > // yes, find the pointer in the vtable >> >> > > else >> >> > > // no, just return the pointer >> >> > > so ideally we want to evaluate the then-branch. Eventually it'll evaluate it >> >> > > to something like _ZTV2X2[2], but the vtable isn't constexpr so we'd end up >> >> > > with "not a constant expression" error. >> >> > >> >> > Then let's mark the vtable as constexpr, there's no reason for it not to be. >> > >> > Done. But then I had to add indexes to the vtable's ctor (because find_array_ctor_elt >> > expects it), which broke an assert in gimple_get_virt_method_for_vtable. But I >> > don't need the array_ref hack anymore! >> >> > Also, I had to set DECL_DECLARED_CONSTEXPR_P after maybe_commonize_var, >> > otherwise we run into the sorry in that function with -fno-weak... >> >> Hmm, we shouldn't give that sorry for DECL_ARTIFICIAL variables. >> >> Looking more closely, it seems that the call to maybe_commonize_var >> from initialize_artificial_var did nothing before this change, since >> the vtable is DECL_ARTIFICIAL, so it didn't pass the condition at the >> top. I suppose we should extend the !DECL_ARTIFICIAL check in >> maybe_commonize_var to the inline variable case as well. > > Done. And then I could move setting DECL_DECLARED_CONSTEXPR_P to > initialize_artificial_var, which is where I think it belongs. > >> >> Ok, unfortunately it wasn't as easy as merely marking it DECL_DECLARED_CONSTEXPR_P >> >> in initialize_artificial_var because then I saw "used in its own initializer" >> >> error. Which I don't know why, but now that I know you agree with this direction >> >> I can dig deeper. >> >> >> >> > > Since the vtable initializer is >> >> > > a compile-time constant, I thought we could make it work by a hack as the one >> >> > > in cxx_eval_array_reference. We'll then let cxx_eval_call_expression do its >> >> > > job and everything is hunky-dory. >> >> > > >> >> > > Except when it isn't: I noticed that the presence of _vptr doesn't make the >> >> > > class non-empty, and when cxx_eval_constant_expression saw a decl with an empty >> >> > > class type, it just evaluated it to { }. But such a class still had gotten an >> >> > > initializer that looks like {.D.2082 = {._vptr.X2 = &_ZTV2X2 + 16}}. So >> >> > > replacing it with { } will lose the proper initializer whereupon we fail. >> >> > > The check I've added to cxx_eval_constant_expression won't win any beauty >> >> > > contests but unfortunately EMPTY_CONSTRUCTOR_P doesn't work there. >> >> > >> >> > Perhaps we should check !TYPE_POLYMORPHIC_P as well as >> >> > is_really_empty_class. Perhaps there should be a predicate for that, >> >> > say, is_really_nearly_empty_class... >> > >> > For now I've only added the !TYPE_POLYMORPHIC_P check, which works just fine. >> > >> > Bootstrapped/regtested on x86_64-linux, ok for trunk? >> > >> > 2018-09-17 Marek Polacek >> > >> > P1064R0 - Allowing Virtual Function Calls in Constant Expressions >> > * call.c (build_over_call): Add FIXME. >> > * class.c (initialize_vtable): Mark the vtable as constexpr. >> > (build_vtbl_initializer): Build vtable's constructor with indexes. >> > * constexpr.c (cxx_eval_constant_expression): Don't ignore _vptr's >> > initializer. Handle OBJ_TYPE_REF. >> > (potential_constant_expression_1): Handle OBJ_TYPE_REF. >> > * decl.c (grokdeclarator): Change error to pedwarn. Only warn when >> > pedantic and not C++2a. >> > >> > * gimple-fold.c (gimple_get_virt_method_for_vtable): Remove assert. >> > >> > * g++.dg/cpp0x/constexpr-virtual5.C: Adjust dg-error. >> > * g++.dg/cpp2a/constexpr-virtual1.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual2.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual3.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual4.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual5.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual6.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual7.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual8.C: New test. >> > * g++.dg/cpp2a/constexpr-virtual9.C: New test. >> > * g++.dg/diagnostic/virtual-constexpr.C: Skip for C++2a. Use >> > -pedantic-errors. Adjust dg-error. >> > >> > diff --git gcc/cp/call.c gcc/cp/call.c >> > index 69503ca7920..6c70874af40 100644 >> > --- gcc/cp/call.c >> > +++ gcc/cp/call.c >> > @@ -8401,7 +8401,8 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) >> > >> > if (DECL_VINDEX (fn) && (flags & LOOKUP_NONVIRTUAL) == 0 >> > /* Don't mess with virtual lookup in instantiate_non_dependent_expr; >> > - virtual functions can't be constexpr. */ >> > + virtual functions can't be constexpr. FIXME Actually, no longer >> > + true in C++2a. */ >> > && !in_template_function ()) >> >> I notice that removing the in_template_function check doesn't break >> template/virtual4.C nowadays. Does it break anything else? > > It doesn't seem to! So, removed. > >> > + /* The C++ FE now produces indexed fields but we can index the array >> > + directly. */ >> > if (access_index < CONSTRUCTOR_NELTS (init)) >> > { >> > fn = CONSTRUCTOR_ELT (init, access_index)->value; >> > - gcc_checking_assert (!CONSTRUCTOR_ELT (init, access_index)->index); >> >> Rather than remove this assert, let's fix it to check that the ->index >> matches access_index. > > Done. > > Bootstrapped/regtested on ppc64le-linux, additionally tested dg.exp with > GXX_TESTSUITE_STDS=98,11,14,17,2a on x86_64-linux, ok for trunk? > > 2018-09-18 Marek Polacek > > P1064R0 - Allowing Virtual Function Calls in Constant Expressions > * call.c (build_over_call): No longer check if we're outside a template > function. > * class.c (build_vtbl_initializer): Build vtable's constructor with > indexes. > * constexpr.c (cxx_eval_constant_expression): Don't ignore _vptr's > initializer. Handle OBJ_TYPE_REF. > (potential_constant_expression_1): Handle OBJ_TYPE_REF. > * decl.c (maybe_commonize_var): Bail out for any DECL_ARTIFICIAL. > (initialize_artificial_var): Mark the variable as constexpr. > (grokdeclarator): Change error to pedwarn. Only warn when > pedantic and not C++2a. > > * gimple-fold.c (gimple_get_virt_method_for_vtable): Adjust assert. > > * g++.dg/cpp0x/constexpr-virtual5.C: Adjust dg-error. > * g++.dg/cpp2a/constexpr-virtual1.C: New test. > * g++.dg/cpp2a/constexpr-virtual2.C: New test. > * g++.dg/cpp2a/constexpr-virtual3.C: New test. > * g++.dg/cpp2a/constexpr-virtual4.C: New test. > * g++.dg/cpp2a/constexpr-virtual5.C: New test. > * g++.dg/cpp2a/constexpr-virtual6.C: New test. > * g++.dg/cpp2a/constexpr-virtual7.C: New test. > * g++.dg/cpp2a/constexpr-virtual8.C: New test. > * g++.dg/cpp2a/constexpr-virtual9.C: New test. > * g++.dg/diagnostic/virtual-constexpr.C: Skip for C++2a. Use > -pedantic-errors. Adjust dg-error. > > diff --git gcc/cp/call.c gcc/cp/call.c > index 69503ca7920..ddf0ed044a0 100644 > --- gcc/cp/call.c > +++ gcc/cp/call.c > @@ -8399,10 +8399,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL) > maybe_warn_class_memaccess (input_location, fn, args); > > - if (DECL_VINDEX (fn) && (flags & LOOKUP_NONVIRTUAL) == 0 > - /* Don't mess with virtual lookup in instantiate_non_dependent_expr; > - virtual functions can't be constexpr. */ > - && !in_template_function ()) > + if (DECL_VINDEX (fn) && (flags & LOOKUP_NONVIRTUAL) == 0) > { > tree t; > tree binfo = lookup_base (TREE_TYPE (TREE_TYPE (argarray[0])), > diff --git gcc/cp/class.c gcc/cp/class.c > index e950a7423f7..b9cde037337 100644 > --- gcc/cp/class.c > +++ gcc/cp/class.c > @@ -9266,6 +9266,7 @@ build_vtbl_initializer (tree binfo, > tree vcall_index; > tree fn, fn_original; > tree init = NULL_TREE; > + tree idx = build_int_cst (size_type_node, jx++); Let's use size_int here, like in process_init_constructor_array. OK with that change. Jason