From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27015 invoked by alias); 16 Aug 2019 00:21:31 -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 26995 invoked by uid 89); 16 Aug 2019 00:21:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=H*Ad:U*jason X-HELO: mail-qk1-f169.google.com Received: from mail-qk1-f169.google.com (HELO mail-qk1-f169.google.com) (209.85.222.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Aug 2019 00:21:29 +0000 Received: by mail-qk1-f169.google.com with SMTP id s145so3339181qke.7 for ; Thu, 15 Aug 2019 17:21:28 -0700 (PDT) Return-Path: Received: from [192.168.1.116] (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 i62sm2207431qke.52.2019.08.15.17.21.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 15 Aug 2019 17:21:26 -0700 (PDT) Subject: Re: C++ PATCH for c++/91264 - detect modifying const objects in constexpr To: Marek Polacek Cc: GCC Patches References: <20190731192659.GP32749@redhat.com> <902366c6-754a-de65-f78e-25834263ac8a@redhat.com> <20190806192021.GL28284@redhat.com> <68bb270b-fa29-972d-7cc3-790dbcf02767@redhat.com> <20190808192555.GY28284@redhat.com> <20190815213456.GS14737@redhat.com> From: Jason Merrill Message-ID: <3bc6420a-b1c4-1be6-aa72-5f27fd768430@redhat.com> Date: Fri, 16 Aug 2019 00:28:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190815213456.GS14737@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg01141.txt.bz2 On 8/15/19 5:34 PM, Marek Polacek wrote: > On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote: >> On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek wrote: >>> >>> On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote: >>>> On 8/6/19 3:20 PM, Marek Polacek wrote: >>>>> On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote: >>>>>> On 7/31/19 3:26 PM, Marek Polacek wrote: >>>>>>> One of the features of constexpr is that it doesn't allow UB; and such UB must >>>>>>> be detected at compile-time. So running your code in a context that requires >>>>>>> a constant expression should ensure that the code in question is free of UB. >>>>>>> In effect, constexpr can serve as a sanitizer. E.g. this article describes in >>>>>>> in more detail: >>>>>>> >>>>>>> >>>>>>> [dcl.type.cv]p4 says "Any attempt to modify a const object during its lifetime >>>>>>> results in undefined behavior." However, as the article above points out, we >>>>>>> aren't detecting that case in constexpr evaluation. >>>>>>> >>>>>>> This patch fixes that. It's not that easy, though, because we have to keep in >>>>>>> mind [class.ctor]p5: >>>>>>> "A constructor can be invoked for a const, volatile or const volatile object. >>>>>>> const and volatile semantics are not applied on an object under construction. >>>>>>> They come into effect when the constructor for the most derived object ends." >>>>>>> >>>>>>> I handled this by keeping a hash set which tracks objects under construction. >>>>>>> I considered other options, such as going up call_stack, but that wouldn't >>>>>>> work with trivial constructor/op=. It was also interesting to find out that >>>>>>> the definition of TREE_HAS_CONSTRUCTOR says "When appearing in a FIELD_DECL, >>>>>>> it means that this field has been duly initialized in its constructor" though >>>>>>> nowhere in the codebase do we set TREE_HAS_CONSTRUCTOR on a FIELD_DECL as far >>>>>>> as I can see. Unfortunately, using this bit proved useless for my needs here. >>>>>> >>>>>>> Also, be mindful of mutable subobjects. >>>>>>> >>>>>>> Does this approach look like an appropriate strategy for tracking objects' >>>>>>> construction? >>>>>> >>>>>> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR >>>>>> to distinguish between initialization and modification; for class objects, I >>>>> >>>>> This is already true: only class object go into the hash set. >>>>> >>>>>> wonder about setting a flag on the CONSTRUCTOR after initialization is >>>>>> complete to indicate that the value is now constant. >>>>> >>>>> But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with >>>>> TREE_CODE == CONSTRUCTOR). We have a CALL_EXPR like Y::Y ((struct Y *) &y), >>>>> which initializes the object "y". Setting a flag on the CALL_EXPR or its underlying >>>>> function decl wouldn't help. >>>>> >>>>> Am I missing something? >>>> >>>> I was thinking that where in your current patch you call >>>> remove_object_under_construction, we could instead mark the object's value >>>> CONSTRUCTOR as immutable. >>> >>> Ah, what you meant was to look at DECL_INITIAL of the object we're >>> constructing, which could be a CONSTRUCTOR. Unfortunately, this >>> DECL_INITIAL is null (in all the new tests when doing >>> remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/. >> >> There's a value in ctx->values, isn't there? > > Doesn't seem to be the case for e.g. > > struct A { > int n; > constexpr A() : n(1) { n = 2; } > }; > > struct B { > const A a; > constexpr B(bool b) { > if (b) > const_cast(a).n = 3; // { dg-error "modifying a const object" } > } > }; > > constexpr B b(false); > static_assert(b.a.n == 2, ""); > > Here we're constructing "b", its ctx->values->get(new_obj) is initially > "{}". In the middle of constructing "b", we construct "b.a", but that > has nothing in ctx->values. Right, subobjects aren't in ctx->values. In cxx_eval_call_expression we have if (DECL_CONSTRUCTOR_P (fun)) /* This can be null for a subobject constructor call, in which case what we care about is the initialization side-effects rather than the value. We could get at the value by evaluating *this, but we don't bother; there's no need to put such a call in the hash table. */ result = lval ? ctx->object : ctx->ctor; Your patch already finds *this (b.a) and puts it in new_obj; if it's const we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on. Jason