On Mon, Jun 26, 2023 at 03:37:32PM -0400, Patrick Palka wrote:
> On Sun, 25 Jun 2023, Nathaniel Shead wrote:
>
> > 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 <nathanielosh...@gmail.com>
> > > > ---
> > > > 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.
>
> Ouch, good catch..
>
> >
> > 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.
>
> This situation seems similar to that of a constexpr call returning a delete'd
> pointer, which we handle by preventing caching of the call:
>
> From constexpr.cc (cxx_eval_call_expression):
> 3207 /* Also don't cache a call that returns a deallocated pointer.
> */
> 3208 if (cacheable && (cp_walk_tree_without_duplicates
> 3209 (&result, find_heap_var_refs, NULL)))
> 3210 cacheable = false;
>
> Maybe we could also disable caching in this situation as well, i.e. whenever a
> constexpr call returns a reference to an expired variable?
Thanks for the pointer, I tried this and it works well. I was initially
a little worried about something similar happening with parameters but
it looks like that's already handled, since non-constant arguments also
already disable caching.
I'm still bootstrapping/regtesting but I'll send out a new version of
this patch series tomorrow when it's done.
> >
> > > > 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<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;
> > > > + 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
> > > >
> > > >
> > >
> >
> >
>
