Re: [PATCH v3 1/3] c++: Track lifetimes in constant evaluation [PR70331,PR96630,PR98675]

[Nathaniel Shead <nathanieloshead@gmail.com>]

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.