From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by sourceware.org (Postfix) with ESMTPS id 027EF3858C50 for ; Sun, 16 Jul 2023 13:47:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 027EF3858C50 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-66872d4a141so2319861b3a.1 for ; Sun, 16 Jul 2023 06:47:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689515249; x=1692107249; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=HHsknycH7eG1ackk+CPKTCegjCR25C6HldMy5jN0zjc=; b=HgQ2MbDWO91y13QFvsrN9tATnPuFBCt3nQJDSStudamLuSlNWsssF1TdBr1dO9D6tp uB8lw25Qec9QrYIIR43Q6GTH7a6ZvM1OoKgRo16gA76Tw+AkyeuLh/yQwq50PRV9d+nC CaalYOkBI6bPMxjXUEZmdLL3wYVMWH6LZ1Sxv9JW8yr+XavH3S3m45hsjZzIMijf13JJ Gv2gEPSqzLsCSweFg51z+SWnhaduHXJspVRMN8OBcZ9IuinV7dqLzWhUTS7o26XtXNDc CC/ORGRM9FQ2REXXDoniRy3d2yGq8B+hQSeIc4UD+QOgCtxV6zhJB/Q0P333a8NfNbr4 RPGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689515249; x=1692107249; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=HHsknycH7eG1ackk+CPKTCegjCR25C6HldMy5jN0zjc=; b=gPUiPWVhzeKMDfW+dWibfaH0WPfJkpakRjtXNePeLNfWqycgJYVjjX77KgvuRcZQ4m RlqlzK3pOVIDbTwoBXiMntMrEWODReRYY5lRG+JhrOw8dtLnxzpJdTQCxsUK5k59mEOG gRcK+cFzd3PB3Rqbph7b2aVQZI7XbipAm0y1I8553cibCugPOicGIog4kP6j1DkhR61j s/L/AKHfPzGwvYqcLztltKxwZ28VRnQnEgBgkNjr+9nDcHpPZDAFq/Nz4oPS+Yzzzkqp pkVO3jfUSGJYJTMf9ozmfM8xIgo5saoqK+qDn72ShBvDy4KdhgI0PATy23LFaT6dxwSo ezXg== X-Gm-Message-State: ABy/qLaD/IvKxNtYVLasBPsX9R15rKOP30q+832OSG/xFK7FGxvUlIIt wWqeRO8VVWm24zrjknjuMPM= X-Google-Smtp-Source: APBJJlHdU23LJq/EQW6bNFIeEKXW6nw1Qq4ZNqhKn8EZjS8vULwZ//80yegPDQ4zcpYXemfB3N1Whg== X-Received: by 2002:a05:6a00:14c6:b0:682:7d8a:f887 with SMTP id w6-20020a056a0014c600b006827d8af887mr10506908pfu.30.1689515248655; Sun, 16 Jul 2023 06:47:28 -0700 (PDT) Received: from Thaum.localdomain (59-102-120-25.tpgi.com.au. [59.102.120.25]) by smtp.gmail.com with ESMTPSA id a4-20020aa780c4000000b006589cf6d88bsm10546562pfn.145.2023.07.16.06.47.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 16 Jul 2023 06:47:28 -0700 (PDT) Date: Sun, 16 Jul 2023 23:47:21 +1000 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Patrick Palka Subject: Re: [PATCH v3 1/3] c++: Track lifetimes in constant evaluation [PR70331,PR96630,PR98675] Message-ID: References: <1abfe7ac-1d9e-da4c-3f09-9e446b7ca3ff@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1abfe7ac-1d9e-da4c-3f09-9e446b7ca3ff@redhat.com> X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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, Jul 14, 2023 at 11:16:58AM -0400, Jason Merrill wrote: > On 6/30/23 23:28, Nathaniel Shead via Gcc-patches wrote: > > This adds rudimentary lifetime tracking in C++ constexpr contexts, > > Thanks! > > I'm not seeing either a copyright assignment or DCO certification for you; > please see https://gcc.gnu.org/contribute.html#legal for more information. > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index cca0435bafc..bc59b4aab67 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -1188,7 +1190,12 @@ public: > > if (!already_in_map && modifiable) > > modifiable->add (t); > > } > > - void remove_value (tree t) { values.remove (t); } > > + void remove_value (tree t) > > + { > > + if (DECL_P (t)) > > + outside_lifetime.add (t); > > + values.remove (t); > > What if, instead of removing the variable from one hash table and adding it > to another, we change the value to, say, void_node? I have another patch I'm working on after this which does seem to require the overlapping tables to properly catch uses of aggregates while they are still being constructed (i.e. before their lifetime has begun), as part of PR c++/109518. In that case the 'values' map contains the CONSTRUCTOR node for the aggregate, but it also needs to be in 'outside_lifetime'. I could also explore solving this another way however if you prefer. (I also have vague dreams of at some point making this a map to the location that the object was destroyed for more context in the error messages, but I'm not yet sure if that's feasible or will actually be all that helpful so I'm happy to forgo that.) > > + /* Also don't cache a call if we return a pointer to an expired > > + value. */ > > + if (cacheable && (cp_walk_tree_without_duplicates > > + (&result, find_expired_values, > > + &ctx->global->outside_lifetime))) > > + cacheable = false; > > I think we need to reconsider cacheability in general; I think we only want > to cache calls that are themselves valid constant expressions, in that the > return value is a "permitted result of a constant expression" > (https://eel.is/c++draft/expr.const#13). A pointer to an automatic variable > is not, whether or not it is currently within its lifetime. > > That is, only cacheable if reduced_constant_expression_p (result). > > I'm experimenting with this now, you don't need to mess with it. Thanks! I agree, that sounds a lot nicer; I definitely ran into caching problems in a few different ways when I was developing this patch, and this approach sounds like it would have avoided that. > > @@ -7085,7 +7138,7 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > > case PARM_DECL: > > if (lval && !TYPE_REF_P (TREE_TYPE (t))) > > /* glvalue use. */; > > - else if (tree v = ctx->global->get_value (r)) > > + else if (tree v = ctx->global->get_value (t)) > > I agree with this change, but it doesn't have any actual effect, right? I'll > go ahead and apply it separately. Yup, it was just a drive-by cleanup I made while trying to understand this part of the code. Thanks. > > @@ -7328,17 +7386,28 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > > auto_vec cleanups; > > vec *prev_cleanups = ctx->global->cleanups; > > ctx->global->cleanups = &cleanups; > > - r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), > > + > > + auto_vec save_exprs; > > Now that we're going to track temporaries for each full-expression, I think > we shouldn't also need to track them for loops and calls. Good point, I didn't think about that. I'm now bootstrapping/regtesting a modification of this patch that removes the tracking in loops and calls, but an initial run of the dg.exp testsuite is promising. It also fixes an issue I just noticed where I don't actually check lifetimes of empty types. I'll send out a new version when that finishes.