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<tree, 2> cleanups; > > vec<tree> *prev_cleanups = ctx->global->cleanups; > > ctx->global->cleanups = &cleanups; > > - r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), > > + > > + auto_vec<tree, 10> 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.
