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 829B6395A461 for ; Tue, 10 Mar 2020 19:46:18 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583869578; 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=tQRij2Vh0zDmQR8dRegADzJga7wv4JXQDMlsZ3NUqKM=; b=ZNuTjpi82bYCdeXiRLfJTZHvW4aLrjvG1eriw9DVIJdFO+Bdan4D30pPkDw2UP15n6qWBG hldSlEl7v1+63Fa7Ct8w9VyXA1TcTCUbECmfIY5oI4e5NcHwY6u5uE8QR9rviGcQL5yi0e 9qWx4j4FbmwIsBmT0hnrO5YDmbZevBc= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-363-D6ds96c1N5iYXPZLDGHKig-1; Tue, 10 Mar 2020 15:46:12 -0400 X-MC-Unique: D6ds96c1N5iYXPZLDGHKig-1 Received: by mail-qk1-f200.google.com with SMTP id x21so10488446qkn.18 for ; Tue, 10 Mar 2020 12:46:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=tQRij2Vh0zDmQR8dRegADzJga7wv4JXQDMlsZ3NUqKM=; b=BLb2qMw0VuqtTJq8bKXeNSRQic2AOOhcqGD/3IoMvAR4R/6kj+XDz3398MkXlHEliC VT7AYFcPmoxOywrIHFof+/xZvw8hv7nT/7goiCXrXX4cTlaWWirYwzt2LgOavyysnVeP VKvTuNyZLmKNU3q5tXqLXhQoXn5c8HtyU9A4A+gy6B9iwxSjhDtSA9jhcUNaz64QW+M7 WnHnDWChQih+C9LIXPJvoBxIJvb/7E/X1y5gVbxUZnW3TsWj9EcrfDPnG8ldaT94spUL QufsrieWnIc5lFZBldNupAsIvLnapYBqmnLd/EnIV+I90wBZXMQZ0gJ7ljxH+4UrQK23 z5ww== X-Gm-Message-State: ANhLgQ1TvxQ7hZxiYFe0yFjknJUSHi+Grrpr/OcpEw8VAmvIPK1z/8VM f4VlhL6D3cyrCaQdhcYuG+Nc4kpL4Lp7g2X8W+vDr480hTVq4BEiHu2Rt/i1wupM3XKoYqOzvF9 uOOIpw3nyLKu3jsQ/sw== X-Received: by 2002:a05:620a:134a:: with SMTP id c10mr7467347qkl.188.1583869567070; Tue, 10 Mar 2020 12:46:07 -0700 (PDT) X-Google-Smtp-Source: ADFU+vuY6ZFI7NVL6efUVNxLAmQCbnkbM/znC9JYtWW3DGqLFnZprdjVbAqUbGPBN/EE0an2x3ZsNw== X-Received: by 2002:a05:620a:134a:: with SMTP id c10mr7467305qkl.188.1583869566477; Tue, 10 Mar 2020 12:46:06 -0700 (PDT) Received: from [192.168.1.148] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id j7sm20550949qti.14.2020.03.10.12.46.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 10 Mar 2020 12:46:04 -0700 (PDT) Subject: Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074] To: Marek Polacek Cc: GCC Patches References: <20200306235406.1943931-1-polacek@redhat.com> <20200309125821.GS2156@tucnak> <99292489-87c3-451d-bbb5-fe089ef08984@redhat.com> <20200309134055.GY3554@redhat.com> <0d703936-119f-fb10-f46d-9424450454dc@redhat.com> <20200309202500.GB3554@redhat.com> <20200309203425.GC3554@redhat.com> From: Jason Merrill Message-ID: <93cda23a-ec3f-dd81-47f8-3cf9c848549e@redhat.com> Date: Tue, 10 Mar 2020 15:46:03 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: <20200309203425.GC3554@redhat.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="------------8CDF87C7F22AC041AAC02A5A" X-Spam-Status: No, score=-27.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS 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: Tue, 10 Mar 2020 19:46:19 -0000 This is a multi-part message in MIME format. --------------8CDF87C7F22AC041AAC02A5A Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 3/9/20 4:34 PM, Marek Polacek wrote: > On Mon, Mar 09, 2020 at 04:25:00PM -0400, Marek Polacek wrote: >> On Mon, Mar 09, 2020 at 03:37:56PM -0400, Jason Merrill wrote: >>> On 3/9/20 9:40 AM, Marek Polacek wrote: >>>> On Mon, Mar 09, 2020 at 09:19:30AM -0400, Jason Merrill wrote: >>>>> On 3/9/20 8:58 AM, Jakub Jelinek wrote: >>>>>> On Fri, Mar 06, 2020 at 07:43:43PM -0500, Jason Merrill wrote: >>>>>>> On 3/6/20 6:54 PM, Marek Polacek wrote: >>>>>>>> I got a report that building Chromium fails with the "modifying a const >>>>>>>> object" error. After some poking I realized it's a bug in GCC, not in >>>>>>>> their codebase. >>>>>>>> >>>>>>>> Much like with ARRAY_REFs, which can be const even though the array >>>>>>>> itself isn't, COMPONENT_REFs can be const although neither the object >>>>>>>> nor the field were declared const. So let's dial down the checking. >>>>>>>> Here the COMPONENT_REF was const because of the "const_cast(m)" >>>>>>>> thing -- cxx_eval_component_reference then builds a COMPONENT_REF with >>>>>>>> TREE_TYPE (t). >>>>>>> >>>>>>> What is folding the const into the COMPONENT_REF? >>>>>> >>>>>> cxx_eval_component_reference when it is called on >>>>>> ((const struct array *) this)->elems >>>>>> with /*lval=*/true and lval is true because we are evaluating >>>>>> = (const int &) &((const struct array *) this)->elems[VIEW_CONVERT_EXPR(n)]; >>>>> >>>>> Ah, sure. We're pretty loose with cv-quals in the constexpr code in >>>>> general, so it's probably not worth trying to change that here. Getting >>>>> back to the patch: >>>> >>>> Yes, here the additional const was caused by a const_cast adding a const. >>>> >>>> But this could also happen with wrapper functions like this one from >>>> __array_traits in std::array: >>>> >>>> static constexpr _Tp& >>>> _S_ref(const _Type& __t, std::size_t __n) noexcept >>>> { return const_cast<_Tp&>(__t[__n]); } >>>> >>>> where the ref-to-const parameter added the const. >>>> >>>>>> + if (TREE_CODE (obj) == COMPONENT_REF) >>>>>> + { >>>>>> + tree op1 = TREE_OPERAND (obj, 1); >>>>>> + if (CP_TYPE_CONST_P (TREE_TYPE (op1))) >>>>>> + return true; >>>>>> + else >>>>>> + { >>>>>> + tree op0 = TREE_OPERAND (obj, 0); >>>>>> + /* The LHS of . or -> might itself be a COMPONENT_REF. */ >>>>>> + if (TREE_CODE (op0) == COMPONENT_REF) >>>>>> + op0 = TREE_OPERAND (op0, 1); >>>>>> + return CP_TYPE_CONST_P (TREE_TYPE (op0)); >>>>>> + } >>>>>> + } >>>>> >>>>> Shouldn't this be a loop? >>>> >>>> I don't think so, though my earlier patch had a call to >>>> >>>> +static bool >>>> +cref_has_const_field (tree ref) >>>> +{ >>>> + while (TREE_CODE (ref) == COMPONENT_REF) >>>> + { >>>> + if (CP_TYPE_CONST_P (TREE_TYPE (TREE_OPERAND (ref, 1)))) >>>> + return true; >>>> + ref = TREE_OPERAND (ref, 0); >>>> + } >>>> + return false; >>>> +} >>> >>>> here. A problem arised when I checked even the outermost expression (which is not a >>>> field_decl), then I saw another problematical error. >>>> >>>> The more outer fields are expected to be checked in subsequent calls to >>>> modifying_const_object_p in next iterations of the >>>> >>>> 4459 for (tree probe = target; object == NULL_TREE; ) >>>> >>>> loop in cxx_eval_store_expression. >>> >>> OK, but then why do you want to check two levels here rather than just one? >> >> It's a hack to keep constexpr-tracking-const7.C working. There we have >> >> b.a.c.d.n >> >> wherein 'd' is const struct D, but 'n' isn't const. Without the hack >> const_object_being_modified would be 'b.a.c.d', but due to the problem I >> desribed in the original mail[1] the constructor for D wouldn't have >> TREE_READONLY set. With the hack const_object_being_modified will be >> 'b.a.c.d.n', which is of non-class type so we error: >> >> 4710 if (!CLASS_TYPE_P (const_objtype)) >> 4711 fail = true; >> >> I could remove the hack and maybe XFAIL constexpr-tracking-const7.C if you >> want. Unfortunately I wasn't aware of [1] when I added that feature and >> checking if the whole COMPONENT_REF is const seemed to be enough. So if D was a wrapper around another class with the int field, this hack looking one level out wouldn't help? >> It's probably not a good idea to make this checking more strict at this >> stage. >> >> [1] "While looking into this I noticed that we don't detect modifying a const >> object in certain cases like in >> . That's because >> we never evaluate an X::X() CALL_EXPR -- there's none. So there's no >> CONSTRUCTOR to set TREE_READONLY on. No idea how to fix this, but it's >> likely something for GCC 11 anyway." How about this? --------------8CDF87C7F22AC041AAC02A5A Content-Type: text/x-patch; charset=UTF-8; name="const.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="const.diff" diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 76af0d710c4..b3d3499b9ac 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4759,6 +4759,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, else *valp = init; + /* After initialization, 'const' semantics apply to the value of the + object. Make a note of this fact by marking the CONSTRUCTOR + TREE_READONLY. */ + if (TREE_CODE (t) == INIT_EXPR + && TREE_CODE (*valp) == CONSTRUCTOR + && TYPE_READONLY (type)) + TREE_READONLY (*valp) = true; + /* Update TREE_CONSTANT and TREE_SIDE_EFFECTS on enclosing CONSTRUCTORs, if any. */ tree elt; --------------8CDF87C7F22AC041AAC02A5A--