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.129.124]) by sourceware.org (Postfix) with ESMTPS id C7A513858C52 for ; Fri, 3 Feb 2023 18:53:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C7A513858C52 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=1675450432; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=L7pH3TndmUfKkjEikNJe0uu9e7XJB/9+FFIM/lSIu9I=; b=ifrT1SoWiWlQ4Zfuzu1Igw3027JMuX2fGcRbZe/z/XnFheEunhTwR3qNTsLx4h9kCdRwZP 2bV/gkzVCaeqfEiWJdZYA+5c/2uj2pNZJKi6rYRAb4DXz/nZ0OejcqRQFGFQrCiFbZzW1+ 65IFM5DWgEhPY9+ZI8WoHxO2FRWiFu0= 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_128_GCM_SHA256) id us-mta-607-knkMi_4_MNyTcWfowuUuTw-1; Fri, 03 Feb 2023 13:53:51 -0500 X-MC-Unique: knkMi_4_MNyTcWfowuUuTw-1 Received: by mail-qv1-f70.google.com with SMTP id k15-20020a0cd68f000000b00535261af1b1so3254116qvi.13 for ; Fri, 03 Feb 2023 10:53:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=L7pH3TndmUfKkjEikNJe0uu9e7XJB/9+FFIM/lSIu9I=; b=b6mMAPyjeuqRRxtohZYgCR7Z0IJ5ZuNi+G4VjwB/UtVcdPbqpN1AnV0We4ofIn5q4/ OEyY4NqQl8P5DBbiAuTL7LblstBK/tlRWdYSUXwxrzZDRH40GOJS24/Y7V062NG/Vf+w oCYL1Lu8v3K7sFGISOoBcrgO6JM+76B6cMM88EUXPkQ7dmrDbMObkDYu+vi+42rlM2Ku kV4HProtLAEq/mhi5uLTs24euRkZ5rueMRcEQVXgMx3dL5vB85eEBa0gGzy5RFy1EHN5 DQ0wGIX9uiKr84Mw0B97GJ+MhsZ5GmdsfaofEWhCzcy3XcMwOOLcrMZd562nLGekfKVY CUXw== X-Gm-Message-State: AO0yUKWMzXwcrosrQeCIweTnUf2GszabE+BoGlj0GyHNnJm44Zk7sP2c wNxIgRz3/JyWOxQEWpFv7l6FKY3WI8A7CbfHmLxi9zSor7oJ1LQJyr1g5Gvo+ALCJA3Jld4ujNP kCvKbdsOxxllh9Xdegw== X-Received: by 2002:ac8:5a83:0:b0:3b8:4169:2b96 with SMTP id c3-20020ac85a83000000b003b841692b96mr21304692qtc.30.1675450430367; Fri, 03 Feb 2023 10:53:50 -0800 (PST) X-Google-Smtp-Source: AK7set8WzTEMyY7pJ+2in9htJxoxPZUVDBCKYauT+d1jE8zEslMPFsFRiG4TmphqDnZTekl6oAukYA== X-Received: by 2002:ac8:5a83:0:b0:3b8:4169:2b96 with SMTP id c3-20020ac85a83000000b003b841692b96mr21304657qtc.30.1675450429898; Fri, 03 Feb 2023 10:53:49 -0800 (PST) Received: from [192.168.1.108] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id s26-20020a05622a1a9a00b003b62e8b77e7sm2056099qtc.68.2023.02.03.10.53.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Feb 2023 10:53:49 -0800 (PST) Message-ID: <26eb8d60-3679-2dfc-ed9e-2ae85c1597bf@redhat.com> Date: Fri, 3 Feb 2023 13:53:48 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v2] c++: wrong error with constexpr array and value-init [PR108158] To: Marek Polacek Cc: GCC Patches References: <20230131023549.454983-1-polacek@redhat.com> <48936556-7f40-1b53-672c-b51cac05646f@redhat.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP 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 2/3/23 13:08, Marek Polacek wrote: > On Thu, Feb 02, 2023 at 05:29:44PM -0500, Jason Merrill wrote: >> On 1/30/23 21:35, Marek Polacek wrote: >>> In this test case, we find ourselves evaluating 't' which is >>> ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR(index)] >>> in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so >>> we replace it with 't': >>> >>> new_ctx.object = t; // result_decl replaced >>> >>> and then we go to cxx_eval_constant_expression to evaluate an >>> AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the >>> body of the constructor for seed_or_index): >>> >>> ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0> >>> >>> whereupon in cxx_eval_store_expression we go to the probe loop >>> where the 'this' is evaluated to >>> >>> ze_set.tables_.first_table_.data_[0] >>> >>> so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr >>> so we fail with a bogus error. ze_set is not there because it comes >>> from a different constexpr context (it's not in cv_cache either). >>> >>> The problem started with r12-2304 where I added the new_ctx.object >>> replacement. That was to prevent a type mismatch: the type of 't' >>> and ctx.object were different. >>> >>> It seems clear that we shouldn't have replaced ctx.object here. >>> The cxx_eval_array_reference I mentioned earlier is called from >>> cxx_eval_store_expression: >>> 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue, >>> 6258 non_constant_p, overflow_p); >>> which already created a new context, whose .object we should be >>> using unless, for instance, INIT contained a.b and we're evaluating >>> the 'a' part, which I think was the case for r12-2304; in that case >>> ctx.object has to be something different. >>> >>> A relatively safe fix should be to check the types before replacing >>> ctx.object, as in the below. >> >> Agreed. I'm trying to understand when the replacement could ever make >> sense, since 't' is not the target, it's the initializer. The replacement >> comes from Patrick's fix for 98295, but that testcase no longer hits that >> code (likely due to changes in empty class handling). >> >> If you add a gcc_checking_assert (false) to the replacement, does anything >> trip it? > > It would trip in constexpr-101371.C, added in r12-2304. BUT, and I would > have sworn that it ICEd when I tried, it's not necessary anymore. So it > looks like we can simply remove the new_ctx.object line. At least for > trunk, maybe 12 too. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK, thanks. Let's go with your original patch for 11/12. > -- >8 -- > In this test case, we find ourselves evaluating 't' which is > ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR(index)] > in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so > we replace it with 't': > > new_ctx.object = t; // result_decl replaced > > and then we go to cxx_eval_constant_expression to evaluate an > AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the > body of the constructor for seed_or_index): > > ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0> > > whereupon in cxx_eval_store_expression we go to the probe loop > where the 'this' is evaluated to > > ze_set.tables_.first_table_.data_[0] > > so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr > so we fail with a bogus error. ze_set is not there because it comes > from a different constexpr context (it's not in cv_cache either). > > The problem started with r12-2304 where I added the new_ctx.object > replacement. That was to prevent a type mismatch: the type of 't' > and ctx.object were different. > > It seems clear that we shouldn't have replaced ctx.object here. > The cxx_eval_array_reference I mentioned earlier is called from > cxx_eval_store_expression: > 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue, > 6258 non_constant_p, overflow_p); > which already created a new context, whose .object we should be > using unless, for instance, INIT contained a.b and we're evaluating > the 'a' part, which I think was the case for r12-2304; in that case > ctx.object has to be something different. > > It no longer seems necessary to replace new_ctx.object (likely due to > changes in empty class handling). > > PR c++/108158 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_array_reference): Don't replace > new_ctx.object. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/constexpr-108158.C: New test. > --- > gcc/cp/constexpr.cc | 4 --- > gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C | 32 +++++++++++++++++++ > 2 files changed, 32 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 5b31f9c27d1..564766c8a00 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -4301,10 +4301,6 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > if (!SCALAR_TYPE_P (elem_type)) > { > new_ctx = *ctx; > - if (ctx->object) > - /* If there was no object, don't add one: it could confuse us > - into thinking we're modifying a const object. */ > - new_ctx.object = t; > new_ctx.ctor = build_constructor (elem_type, NULL); > ctx = &new_ctx; > } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C > new file mode 100644 > index 00000000000..e5f5e9954e5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C > @@ -0,0 +1,32 @@ > +// PR c++/108158 > +// { dg-do compile { target c++14 } } > + > +template struct carray { > + T data_[N]{}; > + constexpr T operator[](long index) const { return data_[index]; } > +}; > +struct seed_or_index { > +private: > + long value_ = 0; > +}; > +template struct pmh_tables { > + carray first_table_; > + template > + constexpr void lookup(KeyType, HasherType) const { > + first_table_[0]; > + } > +}; > +template struct unordered_set { > + int equal_; > + carray keys_; > + pmh_tables tables_; > + constexpr unordered_set() : equal_{} {} > + template > + constexpr auto lookup(KeyType key, Hasher hash) const { > + tables_.lookup(key, hash); > + return keys_; > + } > +}; > +constexpr unordered_set<3> ze_set; > +constexpr auto nocount = ze_set.lookup(4, int()); > +constexpr auto nocount2 = unordered_set<3>{}.lookup(4, int()); > > base-commit: c9aef107ce697f58a34734d82f8d2514405c9be0