From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by sourceware.org (Postfix) with ESMTPS id 6BF583858D35 for ; Sat, 24 Jun 2023 14:02:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6BF583858D35 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-pj1-x1036.google.com with SMTP id 98e67ed59e1d1-262d33fa37cso56350a91.3 for ; Sat, 24 Jun 2023 07:02:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687615375; x=1690207375; 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=0DEGKeQcnVba68tiKsWK3zYqGggE00ylB/4TlrIIDkI=; b=EeyG5dKmCIIoVyeR6ljuacu9784rS6kbnoFSoKaqLJ8ZMN9JY3FPHVv1ny3026mEfJ smHelQcn2E9ujJ2OuhOpOAJUUvsSua8WgAuptiRdXd3uasLWLh+q7t4PMphIDFDTB4Mp o6RVupP1/kJ4+DzH0rXuD8HQOab+N5IwahntLo1Gfv4M5htCXPfZ0gUQLG3uFAkXzVNq +9ZcuQ1bmpQN/5vmYNVdRxiCae0Mb1uNpT7NmRvafhAHm4hjrV4I+j1ZfMPQjYD8D2L1 oCLScxc7BDpbXBN0zTvCtyZZtkyVwCgDVtIKOX/J3grn2q9F/GVjLXnnAEhBj+joWNTS PIbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687615375; x=1690207375; 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=0DEGKeQcnVba68tiKsWK3zYqGggE00ylB/4TlrIIDkI=; b=X66oEEXuXK6SclRa7cpLt7BDFSBB6dzq99kM9ivq8f8MO5hvedLCYGFLYQyd7udAod oEbf6S2igPLCbrw/Nmbt2aRpFvSgsJ3avC8VuV7t2wF8xzVhKKhVEtj8bGuknfzZr5v8 CXdVty6zOG0d0xfKFthFZG8ZCfr4h5fsFlFUR+NSqkDSz9x/REzVDsgs0RBre/8Mb4++ M7Xx6TjRBtjS3ypvAblDHjWmdaiHLxYzAVwskWhsnlu+jLt7Knc5KrmzEkZhdfr+DRmg NkLt1ZEfrNlqYAHb0JMUEF0whvePoWC2ptWi8YsZiSn3EpqbUcpV5Gihh0IagKWX2neA xRog== X-Gm-Message-State: AC+VfDxFLntp0Oqnobtyg/xJ84CS5Tdk4djmX55usqxVtd3XSpWKfVPD HhnYwYteV0XS5MVf9R/CZec= X-Google-Smtp-Source: ACHHUZ7118bYZcGhME7UM9CnTfyyx7OiHcE0B+WET5teEMMAp0aVOD1i2yI/1/53+eV+vTlDpIe7xA== X-Received: by 2002:a17:90a:d310:b0:25e:b5ba:8bb9 with SMTP id p16-20020a17090ad31000b0025eb5ba8bb9mr17675154pju.5.1687615374937; Sat, 24 Jun 2023 07:02:54 -0700 (PDT) Received: from Thaum.localdomain (59-102-120-25.tpgi.com.au. [59.102.120.25]) by smtp.gmail.com with ESMTPSA id q9-20020a17090a2e0900b00261139fd67asm3112311pjd.36.2023.06.24.07.02.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 24 Jun 2023 07:02:54 -0700 (PDT) Date: Sun, 25 Jun 2023 00:02:50 +1000 From: Nathaniel Shead To: Patrick Palka Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2 1/3] c++: Track lifetimes in constant evaluation [PR70331, PR96630, PR98675] Message-ID: References: <20230329023258.13487-1-nathanieloshead@gmail.com> <20230329023258.13487-2-nathanieloshead@gmail.com> <75995995-bf20-6644-433e-1b43bc27de09@idea> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <75995995-bf20-6644-433e-1b43bc27de09@idea> X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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, Jun 23, 2023 at 12:43:21PM -0400, Patrick Palka wrote: > On Wed, 29 Mar 2023, Nathaniel Shead via Gcc-patches wrote: > > > This adds rudimentary lifetime tracking in C++ constexpr contexts, > > allowing the compiler to report errors with using values after their > > backing has gone out of scope. We don't yet handle other ways of ending > > lifetimes (e.g. explicit destructor calls). > > Awesome! > > > > > PR c++/96630 > > PR c++/98675 > > PR c++/70331 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (constexpr_global_ctx::put_value): Mark value as > > in lifetime. > > (constexpr_global_ctx::remove_value): Mark value as expired. > > (cxx_eval_call_expression): Remove comment that is no longer > > applicable. > > (non_const_var_error): Add check for expired values. > > (cxx_eval_constant_expression): Add checks for expired values. Forget > > local variables at end of bind expressions. Forget temporaries at end > > of cleanup points. > > * cp-tree.h (struct lang_decl_base): New flag to track expired values > > in constant evaluation. > > (DECL_EXPIRED_P): Access the new flag. > > (SET_DECL_EXPIRED_P): Modify the new flag. > > * module.cc (trees_out::lang_decl_bools): Write out the new flag. > > (trees_in::lang_decl_bools): Read in the new flag. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp0x/constexpr-ice20.C: Update error raised by test. > > * g++.dg/cpp1y/constexpr-lifetime1.C: New test. > > * g++.dg/cpp1y/constexpr-lifetime2.C: New test. > > * g++.dg/cpp1y/constexpr-lifetime3.C: New test. > > * g++.dg/cpp1y/constexpr-lifetime4.C: New test. > > * g++.dg/cpp1y/constexpr-lifetime5.C: New test. > > > > Signed-off-by: Nathaniel Shead > > --- > > gcc/cp/constexpr.cc | 69 +++++++++++++++---- > > gcc/cp/cp-tree.h | 10 ++- > > gcc/cp/module.cc | 2 + > > gcc/testsuite/g++.dg/cpp0x/constexpr-ice20.C | 2 +- > > .../g++.dg/cpp1y/constexpr-lifetime1.C | 13 ++++ > > .../g++.dg/cpp1y/constexpr-lifetime2.C | 20 ++++++ > > .../g++.dg/cpp1y/constexpr-lifetime3.C | 13 ++++ > > .../g++.dg/cpp1y/constexpr-lifetime4.C | 11 +++ > > .../g++.dg/cpp1y/constexpr-lifetime5.C | 11 +++ > > 9 files changed, 137 insertions(+), 14 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime1.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime2.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime3.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime4.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index 3de60cfd0f8..bdbc12144a7 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -1185,10 +1185,22 @@ public: > > void put_value (tree t, tree v) > > { > > bool already_in_map = values.put (t, v); > > + if (!already_in_map && DECL_P (t)) > > + { > > + if (!DECL_LANG_SPECIFIC (t)) > > + retrofit_lang_decl (t); > > + if (DECL_LANG_SPECIFIC (t)) > > + SET_DECL_EXPIRED_P (t, false); > > + } > > Since this new flag would only be used only during constexpr evaluation, > could we instead use an on-the-side hash_set in constexpr_global_ctx for > tracking expired-ness? That way we won't have to allocate a > DECL_LANG_SPECIFIC structure for decls that lack it, and won't have to > worry about the flag in other parts of the compiler. I've tried this but I haven't been able to get it to work well. The main issue I'm running into is the caching of function calls in constant evaluation. For example, consider the following: constexpr const double& test() { const double& local = 3.0; return local; } constexpr int foo(const double&) { return 5; } constexpr int a = foo(test()); static_assert(test() == 3.0); When constant-evaluating 'a', we evaluate 'test()'. It returns a value that ends its lifetime immediately, so we mark this in 'ctx->global' as expired. However, 'foo()' never actually evaluates this expired value, so the initialisation of 'a' succeeds. However, then when the static assertion attempts to constant evaluate a second time, the result of 'test' has already been cached, and we just get directly handed a value. This is a new constant evaluation, so 'ctx->global' has been reset, and because we just got the result of the cached function we don't actually know whether this is expired or not anymore, and so this compiles without any error in case it was valid. I haven't yet been able to come up with a good way of avoiding this issue without complicating the caching of call expressions overly much. I suppose I could add an extra field to 'constexpr_call' to track if the value has already been expired (which would solve this particular case), but I'm worried that I'll overlook other cases that will sidestep this. Do you have any other thoughts on the best approach here? Thanks. > > 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) && DECL_LANG_SPECIFIC (t)) > > + SET_DECL_EXPIRED_P (t, true); > > + values.remove (t); > > + } > > }; > > > > /* Helper class for constexpr_global_ctx. In some cases we want to avoid > > @@ -3157,10 +3169,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > for (tree save_expr : save_exprs) > > ctx->global->remove_value (save_expr); > > > > - /* Remove the parms/result from the values map. Is it worth > > - bothering to do this when the map itself is only live for > > - one constexpr evaluation? If so, maybe also clear out > > - other vars from call, maybe in BIND_EXPR handling? */ > > + /* Remove the parms/result from the values map. */ > > ctx->global->remove_value (res); > > for (tree parm = parms; parm; parm = TREE_CHAIN (parm)) > > ctx->global->remove_value (parm); > > @@ -5708,6 +5717,13 @@ non_const_var_error (location_t loc, tree r, bool fundef_p) > > inform (DECL_SOURCE_LOCATION (r), "allocated here"); > > return; > > } > > + if (DECL_EXPIRED_P (r)) > > + { > > + if (constexpr_error (loc, fundef_p, "accessing object outside its " > > + "lifetime")) > > + inform (DECL_SOURCE_LOCATION (r), "declared here"); > > + return; > > + } > > if (!constexpr_error (loc, fundef_p, "the value of %qD is not usable in " > > "a constant expression", r)) > > return; > > @@ -7048,6 +7064,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > > r = build_constructor (TREE_TYPE (t), NULL); > > TREE_CONSTANT (r) = true; > > } > > + else if (DECL_EXPIRED_P (t)) > > + { > > + if (!ctx->quiet) > > + non_const_var_error (loc, r, /*fundef_p*/false); > > + *non_constant_p = true; > > + break; > > + } > > else if (ctx->strict) > > r = decl_really_constant_value (t, /*unshare_p=*/false); > > else > > @@ -7093,7 +7116,15 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > > else > > { > > if (!ctx->quiet) > > - error ("%qE is not a constant expression", t); > > + { > > + if (DECL_EXPIRED_P (r)) > > + { > > + error_at (loc, "accessing object outside its lifetime"); > > + inform (DECL_SOURCE_LOCATION (r), "declared here"); > > + } > > + else > > + error_at (loc, "%qE is not a constant expression", t); > > + } > > *non_constant_p = true; > > } > > break; > > @@ -7315,17 +7346,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; > > + constexpr_ctx new_ctx = *ctx; > > + new_ctx.save_exprs = &save_exprs; > > + > > + r = cxx_eval_constant_expression (&new_ctx, TREE_OPERAND (t, 0), > > lval, > > non_constant_p, overflow_p, > > jump_target); > > + > > ctx->global->cleanups = prev_cleanups; > > unsigned int i; > > tree cleanup; > > /* Evaluate the cleanups. */ > > FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup) > > - cxx_eval_constant_expression (ctx, cleanup, vc_discard, > > + cxx_eval_constant_expression (&new_ctx, cleanup, vc_discard, > > non_constant_p, overflow_p); > > + > > + /* Forget SAVE_EXPRs and TARGET_EXPRs created by this > > + full-expression. */ > > + for (tree save_expr : save_exprs) > > + ctx->global->remove_value (save_expr); > > } > > break; > > > > @@ -7831,10 +7873,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > > non_constant_p, overflow_p, jump_target); > > > > case BIND_EXPR: > > - return cxx_eval_constant_expression (ctx, BIND_EXPR_BODY (t), > > - lval, > > - non_constant_p, overflow_p, > > - jump_target); > > + r = cxx_eval_constant_expression (ctx, BIND_EXPR_BODY (t), > > + lval, > > + non_constant_p, overflow_p, > > + jump_target); > > + for (tree decl = BIND_EXPR_VARS (t); decl; decl = DECL_CHAIN (decl)) > > + ctx->global->remove_value (decl); > > + return r; > > > > case PREINCREMENT_EXPR: > > case POSTINCREMENT_EXPR: > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index b74c18b03ad..3cc08da816f 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -2860,6 +2860,7 @@ struct GTY(()) lang_decl_base { > > unsigned concept_p : 1; /* applies to vars and functions */ > > unsigned var_declared_inline_p : 1; /* var */ > > unsigned dependent_init_p : 1; /* var */ > > + unsigned expired_p : 1; /* var or parm */ > > > > /* The following apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE > > decls. */ > > @@ -2871,7 +2872,7 @@ struct GTY(()) lang_decl_base { > > /* VAR_DECL or FUNCTION_DECL has keyed decls. */ > > unsigned module_keyed_decls_p : 1; > > > > - /* 12 spare bits. */ > > + /* 11 spare bits. */ > > }; > > > > /* True for DECL codes which have template info and access. */ > > @@ -4366,6 +4367,13 @@ get_vec_init_expr (tree t) > > #define SET_DECL_DEPENDENT_INIT_P(NODE, X) \ > > (DECL_LANG_SPECIFIC (VAR_DECL_CHECK (NODE))->u.base.dependent_init_p = (X)) > > > > +/* Nonzero if NODE is a VAR_DECL, PARM_DECL, or FIELD_DECL that is within > > + its lifetime for constant evaluation purposes. */ > > +#define DECL_EXPIRED_P(NODE) \ > > + (DECL_LANG_SPECIFIC (NODE) && DECL_LANG_SPECIFIC (NODE)->u.base.expired_p) > > +#define SET_DECL_EXPIRED_P(NODE, X) \ > > + (DECL_LANG_SPECIFIC (NODE)->u.base.expired_p = (X)) > > + > > /* Nonzero if NODE is an artificial VAR_DECL for a C++17 structured binding > > declaration or one of VAR_DECLs for the user identifiers in it. */ > > #define DECL_DECOMPOSITION_P(NODE) \ > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > index ac2fe66b080..7af43b5736d 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -5654,6 +5654,7 @@ trees_out::lang_decl_bools (tree t) > > WB (lang->u.base.concept_p); > > WB (lang->u.base.var_declared_inline_p); > > WB (lang->u.base.dependent_init_p); > > + WB (lang->u.base.expired_p); > > /* When building a header unit, everthing is marked as purview, (so > > we know which decls to write). But when we import them we do not > > want to mark them as in module purview. */ > > @@ -5728,6 +5729,7 @@ trees_in::lang_decl_bools (tree t) > > RB (lang->u.base.concept_p); > > RB (lang->u.base.var_declared_inline_p); > > RB (lang->u.base.dependent_init_p); > > + RB (lang->u.base.expired_p); > > RB (lang->u.base.module_purview_p); > > RB (lang->u.base.module_attach_p); > > if (VAR_OR_FUNCTION_DECL_P (t)) > > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ice20.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ice20.C > > index e2d4853a284..ebaa95e5324 100644 > > --- a/gcc/testsuite/g++.dg/cpp0x/constexpr-ice20.C > > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ice20.C > > @@ -4,4 +4,4 @@ > > typedef bool (*Function)(int); > > constexpr bool check(int x, Function p) { return p(x); } // { dg-message "in .constexpr. expansion of" } > > > > -static_assert(check(2, check), ""); // { dg-error "conversion|constant|in .constexpr. expansion of" } > > +static_assert(check(2, check), ""); // { dg-error "conversion|constant|lifetime|in .constexpr. expansion of" } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime1.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime1.C > > new file mode 100644 > > index 00000000000..43aa7c974c1 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime1.C > > @@ -0,0 +1,13 @@ > > +// PR c++/96630 > > +// { dg-do compile { target c++14 } } > > + > > +struct S { > > + int x = 0; > > + constexpr const int& get() const { return x; } > > +}; > > + > > +constexpr const int& test() { > > + auto local = S{}; // { dg-message "note: declared here" } > > + return local.get(); > > +} > > +constexpr int x = test(); // { dg-error "accessing object outside its lifetime" } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime2.C > > new file mode 100644 > > index 00000000000..22cd919fcda > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime2.C > > @@ -0,0 +1,20 @@ > > +// PR c++/98675 > > +// { dg-do compile { target c++14 } } > > + > > +struct S { > > + int x = 0; > > + constexpr const int& get() const { return x; } > > +}; > > + > > +constexpr int error() { > > + const auto& local = S{}.get(); // { dg-message "note: declared here" } > > + return local; > > +} > > +constexpr int x = error(); // { dg-error "accessing object outside its lifetime" } > > + > > +constexpr int ok() { > > + // temporary should only be destroyed after end of full-expression > > + auto local = S{}.get(); > > + return local; > > +} > > +constexpr int y = ok(); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime3.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime3.C > > new file mode 100644 > > index 00000000000..6329f8cf6c6 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime3.C > > @@ -0,0 +1,13 @@ > > +// PR c++/70331 > > +// { dg-do compile { target c++14 } } > > + > > +constexpr int f(int i) { > > + int *p = &i; > > + if (i == 0) { > > + int j = 123; // { dg-message "note: declared here" } > > + p = &j; > > + } > > + return *p; > > +} > > + > > +constexpr int i = f(0); // { dg-error "accessing object outside its lifetime" } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime4.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime4.C > > new file mode 100644 > > index 00000000000..181a1201663 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime4.C > > @@ -0,0 +1,11 @@ > > +// { dg-do compile { target c++14 } } > > + > > +constexpr const double& test() { > > + const double& local = 3.0; // { dg-message "note: declared here" } > > + return local; > > +} > > + > > +static_assert(test() == 3.0, ""); // { dg-error "constant|accessing object outside its lifetime" } > > + > > +// no deference, shouldn't error > > +static_assert((test(), true), ""); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > new file mode 100644 > > index 00000000000..a4bc71d890a > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > @@ -0,0 +1,11 @@ > > +// { dg-do compile { target c++14 } } > > +// { dg-options "-Wno-return-local-addr" } > > + > > +constexpr const int& id(int x) { return x; } > > + > > +constexpr bool test() { > > + const int& y = id(3); > > + return y == 3; > > +} > > + > > +constexpr bool x = test(); // { dg-error "" } > > -- > > 2.34.1 > > > > >