diff mbox series

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

Message ID 20230328113230.19975-2-nathanieloshead@gmail.com
State New
Headers show
Series Track lifetimes in constant evaluation [PR70331,...] | expand

Commit Message

Nathaniel Shead March 28, 2023, 11:32 a.m. UTC 
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).

	PR c++/70331
	PR c++/96630
	PR c++/98675

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.
	* module.cc (trees_out::core_bools): Write out the new flag.
	(trees_in::core_bools): Read in the new flag.

gcc/ChangeLog:

	* tree-core.h (struct tree_decl_common): New flag to check if
        value lifetime has expired.
	* tree.h (DECL_EXPIRED): Access the new flag.
	* print-tree.cc (print_node): Print 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 <nathanieloshead@gmail.com>
---
 gcc/cp/constexpr.cc                           | 66 +++++++++++++++----
 gcc/cp/module.cc                              |  2 +
 gcc/print-tree.cc                             |  4 ++
 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 ++++
 gcc/tree-core.h                               |  5 +-
 gcc/tree.h                                    |  6 ++
 11 files changed, 138 insertions(+), 15 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

Jakub Jelinek March 28, 2023, 11:50 a.m. UTC | #1 
On Tue, Mar 28, 2023 at 10:32:28PM +1100, Nathaniel Shead via Gcc-patches wrote:
> 	* tree-core.h (struct tree_decl_common): New flag to check if
>         value lifetime has expired.
> 	* tree.h (DECL_EXPIRED): Access the new flag.
> 	* print-tree.cc (print_node): Print the new flag.
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1834,7 +1834,10 @@ struct GTY(()) tree_decl_common {
>    /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
>    unsigned int decl_not_flexarray : 1;
>  
> -  /* 13 bits unused.  */
> +  /* In VAR_DECL, PARM_DECL, or RESULT_DECL, this is DECL_EXPIRED.  */
> +  unsigned int decl_expired_flag : 1;
> +
> +  /* 12 bits unused.  */
>  
>    /* UID for points-to sets, stable over copying from inlining.  */
>    unsigned int pt_uid;
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -2697,6 +2697,12 @@ extern tree vector_element_bits_tree (const_tree);
>     ??? Need to figure out some way to check this isn't a PARM_DECL.  */
>  #define DECL_INITIAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.initial)
>  
> +/* Used in a VAR_DECL, PARM_DECL, or RESULT_DECL to indicate whether
> +   this declaration is currently in lifetime for constant evaluation
> +   purposes.  */
> +#define DECL_EXPIRED(NODE) \
> +  (DECL_COMMON_CHECK(NODE)->decl_common.decl_expired_flag)
> +
>  /* Holds the size of the datum, in bits, as a tree expression.
>     Need not be constant and may be null.  May be less than TYPE_SIZE
>     for a C++ FIELD_DECL representing a base class subobject with its

While it is true that tree_decl_common has some spare bits, this support
is for an implementation detail of the C++ FE and as such, I think it is
highly preferred to use some bit in the lang specific data structures
rather than wasting bits that we could need later on for things that will
be needed in the middle-end.
struct lang_decl_base I think also has 12 spare bits...

Rest I'll defer to C++ maintainers (also whether this is appropriate now
or should be deferred for GCC 14 stage1).

	Jakub
Nathaniel Shead March 28, 2023, 12:04 p.m. UTC | #2 
On Tue, Mar 28, 2023 at 10:50 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Mar 28, 2023 at 10:32:28PM +1100, Nathaniel Shead via Gcc-patches wrote:
> >       * tree-core.h (struct tree_decl_common): New flag to check if
> >         value lifetime has expired.
> >       * tree.h (DECL_EXPIRED): Access the new flag.
> >       * print-tree.cc (print_node): Print the new flag.
> > --- a/gcc/tree-core.h
> > +++ b/gcc/tree-core.h
> > @@ -1834,7 +1834,10 @@ struct GTY(()) tree_decl_common {
> >    /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
> >    unsigned int decl_not_flexarray : 1;
> >
> > -  /* 13 bits unused.  */
> > +  /* In VAR_DECL, PARM_DECL, or RESULT_DECL, this is DECL_EXPIRED.  */
> > +  unsigned int decl_expired_flag : 1;
> > +
> > +  /* 12 bits unused.  */
> >
> >    /* UID for points-to sets, stable over copying from inlining.  */
> >    unsigned int pt_uid;
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -2697,6 +2697,12 @@ extern tree vector_element_bits_tree (const_tree);
> >     ??? Need to figure out some way to check this isn't a PARM_DECL.  */
> >  #define DECL_INITIAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.initial)
> >
> > +/* Used in a VAR_DECL, PARM_DECL, or RESULT_DECL to indicate whether
> > +   this declaration is currently in lifetime for constant evaluation
> > +   purposes.  */
> > +#define DECL_EXPIRED(NODE) \
> > +  (DECL_COMMON_CHECK(NODE)->decl_common.decl_expired_flag)
> > +
> >  /* Holds the size of the datum, in bits, as a tree expression.
> >     Need not be constant and may be null.  May be less than TYPE_SIZE
> >     for a C++ FIELD_DECL representing a base class subobject with its
>
> While it is true that tree_decl_common has some spare bits, this support
> is for an implementation detail of the C++ FE and as such, I think it is
> highly preferred to use some bit in the lang specific data structures
> rather than wasting bits that we could need later on for things that will
> be needed in the middle-end.
> struct lang_decl_base I think also has 12 spare bits...
>
> Rest I'll defer to C++ maintainers (also whether this is appropriate now
> or should be deferred for GCC 14 stage1).
>
>         Jakub
>

Thanks, I didn't notice that there were lang specific data structures;
I'll take a look at using that instead.

From my point of view I was expecting this would be too late for GCC13
(especially since it should only affect already invalid code, and the
risk of false positives), but I'm happy either way.
diff mbox series

Patch

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 3de60cfd0f8..28e3f891fb8 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -1185,10 +1185,17 @@  public:
   void put_value (tree t, tree v)
   {
     bool already_in_map = values.put (t, v);
+    if (!already_in_map && DECL_P (t))
+      DECL_EXPIRED (t) = false;
     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_EXPIRED (t) = true;
+    values.remove (t);
+  }
 };
 
 /* Helper class for constexpr_global_ctx.  In some cases we want to avoid
@@ -3157,10 +3164,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 +5712,13 @@  non_const_var_error (location_t loc, tree r, bool fundef_p)
 	inform (DECL_SOURCE_LOCATION (r), "allocated here");
       return;
     }
+  if (DECL_EXPIRED (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;
@@ -6995,7 +7006,7 @@  cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
       else
 	{
 	  if (!ctx->quiet)
-	    error ("%qE is not a constant expression", t);
+	    non_const_var_error (loc, t, /*fundef_p*/false);
 	  *non_constant_p = true;
 	}
       break;
@@ -7048,6 +7059,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 (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 +7111,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 (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 +7341,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 +7868,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/module.cc b/gcc/cp/module.cc
index ac2fe66b080..e8b001424a0 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -5435,6 +5435,7 @@  trees_out::core_bools (tree t)
       WB (t->decl_common.decl_read_flag);
       WB (t->decl_common.decl_nonshareable_flag);
       WB (t->decl_common.decl_not_flexarray);
+      WB (t->decl_common.decl_expired_flag);
     }
 
   if (CODE_CONTAINS_STRUCT (code, TS_DECL_WITH_VIS))
@@ -5580,6 +5581,7 @@  trees_in::core_bools (tree t)
       RB (t->decl_common.decl_read_flag);
       RB (t->decl_common.decl_nonshareable_flag);
       RB (t->decl_common.decl_not_flexarray);
+      RB (t->decl_common.decl_expired_flag);
     }
 
   if (CODE_CONTAINS_STRUCT (code, TS_DECL_WITH_VIS))
diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
index 1f3afcbbc86..ce15f4508d7 100644
--- a/gcc/print-tree.cc
+++ b/gcc/print-tree.cc
@@ -495,6 +495,10 @@  print_node (FILE *file, const char *prefix, tree node, int indent,
 	  && DECL_BY_REFERENCE (node))
 	fputs (" passed-by-reference", file);
 
+      if ((code == VAR_DECL || code == PARM_DECL || code == RESULT_DECL)
+	  && DECL_EXPIRED (node))
+	fputs (" expired", file);
+
       if (CODE_CONTAINS_STRUCT (code, TS_DECL_WITH_VIS)  && DECL_DEFER_OUTPUT (node))
 	fputs (" defer-output", file);
 
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 "" }
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index fd2be57b78c..f814b427a38 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1834,7 +1834,10 @@  struct GTY(()) tree_decl_common {
   /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
   unsigned int decl_not_flexarray : 1;
 
-  /* 13 bits unused.  */
+  /* In VAR_DECL, PARM_DECL, or RESULT_DECL, this is DECL_EXPIRED.  */
+  unsigned int decl_expired_flag : 1;
+
+  /* 12 bits unused.  */
 
   /* UID for points-to sets, stable over copying from inlining.  */
   unsigned int pt_uid;
diff --git a/gcc/tree.h b/gcc/tree.h
index 91375f9652f..194d49ed1f7 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -2697,6 +2697,12 @@  extern tree vector_element_bits_tree (const_tree);
    ??? Need to figure out some way to check this isn't a PARM_DECL.  */
 #define DECL_INITIAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.initial)
 
+/* Used in a VAR_DECL, PARM_DECL, or RESULT_DECL to indicate whether
+   this declaration is currently in lifetime for constant evaluation
+   purposes.  */
+#define DECL_EXPIRED(NODE) \
+  (DECL_COMMON_CHECK(NODE)->decl_common.decl_expired_flag)
+
 /* Holds the size of the datum, in bits, as a tree expression.
    Need not be constant and may be null.  May be less than TYPE_SIZE
    for a C++ FIELD_DECL representing a base class subobject with its