From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106974 invoked by alias); 17 Aug 2019 00:40:47 -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 106965 invoked by uid 89); 17 Aug 2019 00:40:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-HELO: mail-qk1-f193.google.com Received: from mail-qk1-f193.google.com (HELO mail-qk1-f193.google.com) (209.85.222.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 17 Aug 2019 00:40:45 +0000 Received: by mail-qk1-f193.google.com with SMTP id s145so6191539qke.7 for ; Fri, 16 Aug 2019 17:40:45 -0700 (PDT) Return-Path: Received: from [192.168.0.15] (75-172-113-80.tukw.qwest.net. [75.172.113.80]) by smtp.gmail.com with ESMTPSA id j61sm3508502qte.47.2019.08.16.17.40.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Aug 2019 17:40:42 -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> <3bc6420a-b1c4-1be6-aa72-5f27fd768430@redhat.com> <20190816121106.GT14737@redhat.com> From: Jason Merrill Message-ID: Date: Sat, 17 Aug 2019 00:51: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: <20190816121106.GT14737@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg01209.txt.bz2 On 8/16/19 5:11 AM, Marek Polacek wrote: > On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote: >> 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. > > Ah, got it! This patch uses setting TREE_READONLY to achieve what I was after. > I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR. > The additional evaluating will only happen for const-qual objects so I hope not > very often. > > Any further comments? Thanks, > > @@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > + /* At this point, the object's constructor will have run, so > + the object is no longer under construction, and its possible > + 'const' semantics now apply. Make a note of this fact by > + marking the CONSTRUCTOR TREE_READONLY. */ > + if (new_obj > + && CLASS_TYPE_P (TREE_TYPE (new_obj)) > + && CP_TYPE_CONST_P (TREE_TYPE (new_obj))) > + { > + tree *ctor = ctx->values->get (new_obj); I don't think trying ctx->values->get first is a win, let's go straight to cxx_eval_constant_expression. > +/* Return true if we are modifying something that is const during constant > + expression evaluation. CODE is the code of the statement, OBJ is the > + object in question, MUTABLE_P is true if one of the subobjects were > + declared mutable. */ > + > +static bool > +modifying_const_object_p (const constexpr_ctx *ctx, tree_code code, tree obj, > + bool mutable_p) > +{ > + /* If this is initialization, there's no problem. */ > + if (code != MODIFY_EXPR) > + return false; > + > + tree type = TREE_TYPE (obj); > + > + /* [basic.type.qualifier] "A const object is an object of type > + const T or a non-mutable subobject of a const object." */ > + return ((TREE_READONLY (obj) || CP_TYPE_CONST_P (type) > + /* If it's an aggregate and any field is const, then it is > + effectively const. */ > + || (CLASS_TYPE_P (type) && C_TYPE_FIELDS_READONLY (type))) This seems wrong; if one field is const, we can still modify other fields. I don't see a test for that case. > @@ -3783,6 +3885,17 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > { > tree ob = TREE_OPERAND (probe, 0); > tree elt = TREE_OPERAND (probe, 1); > + if (DECL_P (elt) && DECL_MUTABLE_P (elt)) > + mutable_p = true; > + if (evaluated > + && modifying_const_object_p (ctx, TREE_CODE (t), probe, > + mutable_p)) > + { > + if (!ctx->quiet) > + modifying_const_object_error (t, probe); > + *non_constant_p = true; > + return t; > + } What if there's a mutable member further down, i.e. struct A { mutable int i; }; struct B { A a; }; const B b; b.a.i = 42; ? And also... > @@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > + if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p)) > + { > + if (!ctx->quiet) > + modifying_const_object_error (t, object); > + *non_constant_p = true; > + return t; > + } ...we are already collecting the CONSTRUCTORs that we're dealing with in the "ctors" stack, we shouldn't need to evaluate object at this point. I'd expect the topmost class-type CONSTRUCTOR on the stack (if any) to be the one we want to look at. I'd think you could do away with much of modifying_const_object_p. > @@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > non_constant_p, overflow_p); > /* Don't share a CONSTRUCTOR that might be changed. */ > init = unshare_constructor (init); > + /* Remember that a constant object's constructor has already > + ran. */ "has...run" Jason