[v3,1/3] c++: Track lifetimes in constant evaluation [PR70331,PR96630,PR98675]
Checks
Commit Message
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
accessing values outside their lifetime (e.g. following explicit
destructor calls).
PR c++/96630
PR c++/98675
PR c++/70331
gcc/cp/ChangeLog:
* constexpr.cc (constexpr_global_ctx::remove_value): Mark value as
outside lifetime.
(find_expired_values): New function.
(outside_lifetime_error): New function.
(cxx_eval_call_expression): Don't cache calls that return references to
values outside their lifetime.
(cxx_eval_constant_expression): Add checks for out-of-lifetime values.
Forget local variables at end of bind expressions, and temporaries
after cleanup points.
gcc/testsuite/ChangeLog:
* 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 <nathanieloshead@gmail.com>
---
gcc/cp/constexpr.cc | 112 ++++++++++++++----
.../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 ++
6 files changed, 160 insertions(+), 20 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
Comments
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?
> + /* 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.
> @@ -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.
> @@ -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.
Jason
On 7/14/23 11:16, Jason Merrill wrote:
> 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.
Oops, now I see the DCO sign-off, not sure how I was missing it.
Jason
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.
On 7/16/23 09:47, Nathaniel Shead wrote:
> On Fri, Jul 14, 2023 at 11:16:58AM -0400, Jason Merrill wrote:
>>
>> 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'd think to handle this with a flag on the CONSTRUCTOR to indicate that
members with no value are out of lifetime (so, a stronger version of
CONSTRUCTOR_NO_CLEARING that just indicates uninitialized). Currently
all the TREE_LANG_FLAG_* are occupied on CONSTRUCTOR, but there seem to
be plenty of spare bits to add a TREE_LANG_FLAG_7.
It might make sense to access those two flags with accessor functions so
they stay aligned.
> (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.)
Hmm, that sounds convenient for debugging, but affected cases would also
be straightforward to debug by adding a run-time call, so I'm skeptical
it would be worth the overhead for successful compiles.
Jason
@@ -1165,6 +1165,8 @@ public:
hash_set<tree> *modifiable;
/* Number of heap VAR_DECL deallocations. */
unsigned heap_dealloc_count;
+ /* Values that are not within their lifetime. */
+ hash_set<tree> outside_lifetime;
/* Constructor. */
constexpr_global_ctx ()
: constexpr_ops_count (0), cleanups (NULL), modifiable (nullptr),
@@ -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);
+ }
};
/* Helper class for constexpr_global_ctx. In some cases we want to avoid
@@ -2509,6 +2516,22 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call,
return cp_build_addr_expr (obj, complain);
}
+/* Look for expired values in the expression *TP, called through
+ cp_walk_tree. DATA is ctx->global->outside_lifetime. */
+
+static tree
+find_expired_values (tree *tp, int *walk_subtrees, void *data)
+{
+ hash_set<tree> *outside_lifetime = (hash_set<tree> *) data;
+
+ if (TYPE_P (*tp))
+ *walk_subtrees = 0;
+ else if (outside_lifetime->contains (*tp))
+ return *tp;
+
+ return NULL_TREE;
+}
+
/* Data structure used by replace_decl and replace_decl_r. */
struct replace_decl_data
@@ -3160,10 +3183,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);
@@ -3210,13 +3230,20 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
cacheable = false;
}
- /* Rewrite all occurrences of the function's RESULT_DECL with the
- current object under construction. */
- if (!*non_constant_p && ctx->object
- && CLASS_TYPE_P (TREE_TYPE (res))
- && !is_empty_class (TREE_TYPE (res)))
- if (replace_decl (&result, res, ctx->object))
- cacheable = false;
+ /* 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;
+
+ /* Rewrite all occurrences of the function's RESULT_DECL with the
+ current object under construction. */
+ if (!*non_constant_p && ctx->object
+ && CLASS_TYPE_P (TREE_TYPE (res))
+ && !is_empty_class (TREE_TYPE (res)))
+ if (replace_decl (&result, res, ctx->object))
+ cacheable = false;
}
else
/* Couldn't get a function copy to evaluate. */
@@ -5687,6 +5714,25 @@ cxx_eval_indirect_ref (const constexpr_ctx *ctx, tree t,
return r;
}
+/* Complain about R, a DECL that is accessed outside its lifetime. */
+
+static void
+outside_lifetime_error (location_t loc, tree r)
+{
+ if (DECL_NAME (r) == heap_deleted_identifier)
+ {
+ /* Provide a more accurate message for deleted variables. */
+ error_at (loc, "use of allocated storage after deallocation "
+ "in a constant expression");
+ inform (DECL_SOURCE_LOCATION (r), "allocated here");
+ }
+ else
+ {
+ error_at (loc, "accessing object outside its lifetime");
+ inform (DECL_SOURCE_LOCATION (r), "declared here");
+ }
+}
+
/* Complain about R, a VAR_DECL, not being usable in a constant expression.
FUNDEF_P is true if we're checking a constexpr function body.
Shared between potential_constant_expression and
@@ -7054,6 +7100,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
r = build_constructor (TREE_TYPE (t), NULL);
TREE_CONSTANT (r) = true;
}
+ else if (ctx->global->outside_lifetime.contains (t))
+ {
+ if (!ctx->quiet)
+ outside_lifetime_error (loc, t);
+ *non_constant_p = true;
+ break;
+ }
else if (ctx->strict)
r = decl_really_constant_value (t, /*unshare_p=*/false);
else
@@ -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))
r = v;
else if (lval)
/* Defer in case this is only used for its type. */;
@@ -7099,7 +7152,12 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
else
{
if (!ctx->quiet)
- error ("%qE is not a constant expression", t);
+ {
+ if (ctx->global->outside_lifetime.contains (t))
+ outside_lifetime_error (loc, t);
+ else
+ error_at (loc, "%qE is not a constant expression", t);
+ }
*non_constant_p = true;
}
break;
@@ -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;
+ 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;
@@ -7855,10 +7924,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:
new file mode 100644
@@ -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" }
new file mode 100644
@@ -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();
new file mode 100644
@@ -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" }
new file mode 100644
@@ -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), "");
new file mode 100644
@@ -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 "" }