From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 3F6973858CDB for ; Fri, 28 Apr 2023 19:40:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3F6973858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1682710811; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=O6c8xdCcsdY0WKhAxMigQRO5rB29lkukztTPSstDiNU=; b=LmD8Jmtx/qKIT29bgI6zGl3CDH/L9ukjdRmvSar3qy+lr1uOfthjHLX8gUI2fj1YtC1y0q G/HdRMXkKDkC/2JTyA18ovw08x/oP+GnwRN3KT2hyBjXIBZd2mKPCPcha4F7X1rD69qPG1 UMGkLMa/Bi7ufMUzLbz1pkQPtLh21v8= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-460-FUaFPBVVONesPgoGfMZvAw-1; Fri, 28 Apr 2023 15:40:10 -0400 X-MC-Unique: FUaFPBVVONesPgoGfMZvAw-1 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-5ef40d2af43so2237676d6.3 for ; Fri, 28 Apr 2023 12:40:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682710810; x=1685302810; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=O6c8xdCcsdY0WKhAxMigQRO5rB29lkukztTPSstDiNU=; b=IQjTUNFDGtqgYdyEnJct4TdWjt7MKcmHK5FRfzKleD1j3AiEkgbEHfcSOLTz38ciTx +xguPj/ih+wCQyG2nn72aTmZvDtvC/bV7WGiZF417abGBFsxz6M4b693BTq983KBXST5 c744FUG4GQQD8ymjBOphnLD18tw/hKXf1WayaO1zYYl8sjn70f1hZGeqyDR22om4esfG MvihWDtHaYoT9bxRE1YGiY0IoONeuwRFjQlAqUTsaBS1vQr6tXeAqRfDkH8RBJMVUTfM 5jHmqPhcLZOwpLXCZUU9KqBJUGGsgGB5AWjbjgVXxMaKLFHU9PBnkkwjKWtIPBw4aDIe RyDw== X-Gm-Message-State: AC+VfDxqMaoErSfMFC2iP6/QZPe/0v6z6iAu4ZkRMEwFUr7NfM1r9suf gRUpQe14iU9XP3eiKm7jx3ub0axJkqCUavgmw7HapHMhs3uGz4ht4mDQAKl233N15rcPlTKDsRl vnBNgNFApM/6quNhbAQ== X-Received: by 2002:a05:6214:248b:b0:5aa:8e3e:496e with SMTP id gi11-20020a056214248b00b005aa8e3e496emr10803240qvb.34.1682710809833; Fri, 28 Apr 2023 12:40:09 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ76Ifv9E4TYuLFprhVMooBDyPwNoe4e3fXQAqAHWr4dbn4J3ykCuqdUQm5nkTCGW4Q3oR6UIw== X-Received: by 2002:a05:6214:248b:b0:5aa:8e3e:496e with SMTP id gi11-20020a056214248b00b005aa8e3e496emr10803193qvb.34.1682710809451; Fri, 28 Apr 2023 12:40:09 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id v14-20020a0ccd8e000000b0061666563741sm2012875qvm.2.2023.04.28.12.40.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Apr 2023 12:40:09 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 28 Apr 2023 15:40:08 -0400 (EDT) To: Patrick Palka cc: gcc-patches@gcc.gnu.org, jason@redhat.com Subject: Re: [PATCH] c++: RESULT_DECL replacement in constexpr call result [PR105440] In-Reply-To: <8cb1d63e-2afb-df4a-21f2-20756e9630ba@idea> Message-ID: References: <20230428190508.4091082-1-ppalka@redhat.com> <8cb1d63e-2afb-df4a-21f2-20756e9630ba@idea> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-13.9 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_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 28 Apr 2023, Patrick Palka wrote: > On Fri, 28 Apr 2023, Patrick Palka wrote: > > > After mechanically replacing RESULT_DECL within a constexpr call result > > (for sake of RVO), we can in some cases simplify the call result > > further. > > > > In the below testcase the result of get() during evaluation of a's > > initializer is the self-referential CONSTRUCTOR: > > > > {._M_p=(char *) &._M_local_buf} > > > > which after replacing RESULT_DECL with ctx->object (aka *D.2603, where > > the D.2603 temporary points to the current element of _M_elems under > > construction) becomes: > > > > {._M_p=(char *) &D.2603->_M_local_buf} > > > > but what we really want is: > > > > {._M_p=(char *) &a._M_elems[0]._M_local_buf}. > > > > so that the value of _M_p is independent of the value of the mutable > > D.2603 temporary. > > > > So to that end, it seems we should constexpr evaluate the result again > > after RESULT_DECL replacement, which is what this patch implements. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > trunk? > > > > PR libstdc++/105440 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (cxx_eval_call_expression): If any RESULT_DECLs get > > replaced in the call result, try further evaluating the result. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp2a/constexpr-dtor16.C: New test. > > --- > > gcc/cp/constexpr.cc | 12 +++++- > > gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C | 39 +++++++++++++++++++ > > 2 files changed, 49 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index d1097764b10..22a1609e664 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -3213,7 +3213,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > && CLASS_TYPE_P (TREE_TYPE (res)) > > && !is_empty_class (TREE_TYPE (res))) > > if (replace_decl (&result, res, ctx->object)) > > - cacheable = false; > > + { > > + cacheable = false; > > + result = cxx_eval_constant_expression (ctx, result, lval, > > + non_constant_p, > > + overflow_p); > > + } > > } > > else > > /* Couldn't get a function copy to evaluate. */ > > @@ -5988,9 +5993,12 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > object = probe; > > else > > { > > + tree orig_probe = probe; > > probe = cxx_eval_constant_expression (ctx, probe, vc_glvalue, > > non_constant_p, overflow_p); > > evaluated = true; > > + if (orig_probe == target) > > + target = probe; > > Whoops, thanks to an accidental git commit --amend this patch contains > an alternative approach that I considered: in cxx_eval_store_expression, > ensure that we always set ctx->object to a fully reduced result (so > &a._M_elems[0] instead of of *D.2603 in this case), which means later > RESULT_DECL replacement with ctx->object should yield an already reduced > result as well. But with this approach I ran into a bogus "modifying > const object" error on cpp1y/constexpr-tracking-const23.C so I gave up > on it :( Ah, the problem was that later in cxx_eval_store_expression we were suppressing a TREE_READONLY update via pattern matching on 'target', but if we are now updating 'target' to its reduced value the pattern matching needs to consider the shape of the original 'target' instead. Here's an alternative fix for this PR that passes regression testing, not sure which approach would be preferable. PR c++/105440 gcc/cp/ChangeLog: * constexpr.cc (cxx_eval_store_expression): Save the original target in 'orig_target'. Update 'target' after evaluating it in the 'probe' loop. Use 'orig_target' instead of 'target' when appropriate. gcc/testsuite/ChangeLog: * g++.dg/cpp2a/constexpr-dtor16.C: New test. --- gcc/cp/constexpr.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 9dbbf6eec03..2939ac89a98 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -5902,8 +5902,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, /* Just ignore clobbers. */ return void_node; + const tree orig_target = TREE_OPERAND (t, 0); + /* First we figure out where we're storing to. */ - tree target = TREE_OPERAND (t, 0); + tree target = orig_target; maybe_simplify_trivial_copy (target, init); @@ -5993,9 +5995,12 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, object = probe; else { + bool is_target = (probe == target); probe = cxx_eval_constant_expression (ctx, probe, vc_glvalue, non_constant_p, overflow_p); evaluated = true; + if (is_target) + target = probe; if (*non_constant_p) return t; } @@ -6159,7 +6164,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, if (!empty_base && !(same_type_ignoring_top_level_qualifiers_p (initialized_type (init), type))) { - gcc_assert (is_empty_class (TREE_TYPE (target))); + gcc_assert (is_empty_class (TREE_TYPE (orig_target))); empty_base = true; } @@ -6294,14 +6299,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, && TREE_CODE (*valp) == CONSTRUCTOR && TYPE_READONLY (type)) { - if (INDIRECT_REF_P (target) + if (INDIRECT_REF_P (orig_target) && (is_this_parameter - (tree_strip_nop_conversions (TREE_OPERAND (target, 0))))) + (tree_strip_nop_conversions (TREE_OPERAND (orig_target, 0))))) /* We've just initialized '*this' (perhaps via the target constructor of a delegating constructor). Leave it up to the caller that set 'this' to set TREE_READONLY appropriately. */ gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p - (TREE_TYPE (target), type) || empty_base); + (TREE_TYPE (orig_target), type) || empty_base); else TREE_READONLY (*valp) = true; } diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C new file mode 100644 index 00000000000..707a3e025b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C @@ -0,0 +1,39 @@ +// PR c++/105440 +// { dg-do compile { target c++20 } } + +struct basic_string { + char _M_local_buf[32]; + char* _M_p; + constexpr basic_string() : _M_p{_M_local_buf} { } + constexpr void f() { if (_M_p) { } } + constexpr ~basic_string() { if (_M_p) { } } +}; + +template +struct array { + basic_string _M_elems[N]; +}; + +constexpr basic_string get() { return {}; } + +constexpr bool f1() { + array<1> a{get()}; + a._M_elems[0].f(); + + return true; +} + +constexpr bool f2() { + array<2> a2{get(), get()}; + array<3> a3{get(), get(), get()}; + + for (basic_string& e : a2._M_elems) + e.f(); + for (basic_string& e : a3._M_elems) + e.f(); + + return true; +} + +static_assert(f1()); +static_assert(f2()); -- 2.40.1.445.gf85cd430b1 > > Here's the correct patch: > > PR libstdc++/105440 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_call_expression): If any RESULT_DECLs get > replaced in the call result, try further evaluating the result. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp2a/constexpr-dtor16.C: New test. > --- > gcc/cp/constexpr.cc | 7 +++- > gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C | 39 +++++++++++++++++++ > 2 files changed, 45 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index d1097764b10..9dbbf6eec03 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -3213,7 +3213,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > && CLASS_TYPE_P (TREE_TYPE (res)) > && !is_empty_class (TREE_TYPE (res))) > if (replace_decl (&result, res, ctx->object)) > - cacheable = false; > + { > + cacheable = false; > + result = cxx_eval_constant_expression (ctx, result, lval, > + non_constant_p, > + overflow_p); > + } > } > else > /* Couldn't get a function copy to evaluate. */ > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > new file mode 100644 > index 00000000000..707a3e025b1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > @@ -0,0 +1,39 @@ > +// PR c++/105440 > +// { dg-do compile { target c++20 } } > + > +struct basic_string { > + char _M_local_buf[32]; > + char* _M_p; > + constexpr basic_string() : _M_p{_M_local_buf} { } > + constexpr void f() { if (_M_p) { } } > + constexpr ~basic_string() { if (_M_p) { } } > +}; > + > +template > +struct array { > + basic_string _M_elems[N]; > +}; > + > +constexpr basic_string get() { return {}; } > + > +constexpr bool f1() { > + array<1> a{get()}; > + a._M_elems[0].f(); > + > + return true; > +} > + > +constexpr bool f2() { > + array<2> a2{get(), get()}; > + array<3> a3{get(), get(), get()}; > + > + for (basic_string& e : a2._M_elems) > + e.f(); > + for (basic_string& e : a3._M_elems) > + e.f(); > + > + return true; > +} > + > +static_assert(f1()); > +static_assert(f2()); > -- > 2.40.1.445.gf85cd430b1 > > > > if (*non_constant_p) > > return t; > > } > > @@ -6154,7 +6162,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > if (!empty_base && !(same_type_ignoring_top_level_qualifiers_p > > (initialized_type (init), type))) > > { > > - gcc_assert (is_empty_class (TREE_TYPE (target))); > > + gcc_assert (is_empty_class (TREE_TYPE (TREE_OPERAND (t, 0)))); > > empty_base = true; > > } > > > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > > new file mode 100644 > > index 00000000000..707a3e025b1 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C > > @@ -0,0 +1,39 @@ > > +// PR c++/105440 > > +// { dg-do compile { target c++20 } } > > + > > +struct basic_string { > > + char _M_local_buf[32]; > > + char* _M_p; > > + constexpr basic_string() : _M_p{_M_local_buf} { } > > + constexpr void f() { if (_M_p) { } } > > + constexpr ~basic_string() { if (_M_p) { } } > > +}; > > + > > +template > > +struct array { > > + basic_string _M_elems[N]; > > +}; > > + > > +constexpr basic_string get() { return {}; } > > + > > +constexpr bool f1() { > > + array<1> a{get()}; > > + a._M_elems[0].f(); > > + > > + return true; > > +} > > + > > +constexpr bool f2() { > > + array<2> a2{get(), get()}; > > + array<3> a3{get(), get(), get()}; > > + > > + for (basic_string& e : a2._M_elems) > > + e.f(); > > + for (basic_string& e : a3._M_elems) > > + e.f(); > > + > > + return true; > > +} > > + > > +static_assert(f1()); > > +static_assert(f2()); > > -- > > 2.40.1.445.gf85cd430b1 > > > > >