C++ PATCH to fix wrong-code with constexpr call table cache (PR c++/83116)

Message ID 20171218150923.GK2605@redhat.com
State New
Headers show
Series
  • C++ PATCH to fix wrong-code with constexpr call table cache (PR c++/83116)
Related show

Commit Message

Marek Polacek Dec. 18, 2017, 3:09 p.m.
Here the problem was that cxx_eval_call_expression can cache the result of a
constexpr call in constexpr_call_table, but we have to be careful, after
store_init_value the result might be invalid.  So I believe we also have to
clear the constexpr call table.  I've lumped it together with clearing
cv_cache.

Bootstrapped/regtested on x86_64-linux, ok for trunk and 7?

2017-12-18  Marek Polacek  <polacek@redhat.com>

	PR c++/83116
	* constexpr.c (clear_cv_cache): Renamed to ...
	(clear_constexpr_cache): ... this.  Also clear constexpr_call_table.
	(clear_cv_and_fold_caches): Renamed to ...
	(clear_constexpr_and_fold_caches): ... this.
	* cp-tree.h (clear_constexpr_and_fold_caches): Update declaration.
	* decl.c (finish_enum_value_list): Call clear_constexpr_and_fold_caches
	instead of clear_cv_and_fold_caches.
	* typeck2.c (store_init_value): Likewise.

	* g++.dg/cpp1y/constexpr-83116.C: New test.


	Marek

Comments

Jason Merrill Dec. 18, 2017, 6:07 p.m. | #1
On Mon, Dec 18, 2017 at 10:09 AM, Marek Polacek <polacek@redhat.com> wrote:
> Here the problem was that cxx_eval_call_expression can cache the result of a

> constexpr call in constexpr_call_table, but we have to be careful, after

> store_init_value the result might be invalid.  So I believe we also have to

> clear the constexpr call table.  I've lumped it together with clearing

> cv_cache.


Hmm, that seems like a big hammer; the problem isn't that
store_init_value makes the result invalid, it's that the result
calculated during store_init_value (when we can treat the object as
constant) isn't relevant later (when the object is no longer
constant).  So we want to avoid caching when we're called for the
initial value.  Maybe by changing

if (depth_ok && !non_constant_args)

to

if (depth_ok && !non_constant_args && ctx->strict)

?  Does that work?
Marek Polacek Dec. 18, 2017, 7:56 p.m. | #2
On Mon, Dec 18, 2017 at 01:07:18PM -0500, Jason Merrill wrote:
> On Mon, Dec 18, 2017 at 10:09 AM, Marek Polacek <polacek@redhat.com> wrote:

> > Here the problem was that cxx_eval_call_expression can cache the result of a

> > constexpr call in constexpr_call_table, but we have to be careful, after

> > store_init_value the result might be invalid.  So I believe we also have to

> > clear the constexpr call table.  I've lumped it together with clearing

> > cv_cache.

> 

> Hmm, that seems like a big hammer; the problem isn't that

> store_init_value makes the result invalid, it's that the result

> calculated during store_init_value (when we can treat the object as

> constant) isn't relevant later (when the object is no longer

> constant).  So we want to avoid caching when we're called for the

> initial value.  Maybe by changing

> 

> if (depth_ok && !non_constant_args)

> 

> to

> 

> if (depth_ok && !non_constant_args && ctx->strict)

> 

> ?  Does that work?


Yes!  I wish I'd thought of ctx->strict before.  Well, something to remember.

Bootstrapped/regtested on x86_64-linux, ok for trunk/7?

2017-12-18  Marek Polacek  <polacek@redhat.com>

	PR c++/83116
	* constexpr.c (cxx_eval_call_expression): Only look into
	constexpr_call_table if ctx->strict.

	* g++.dg/cpp1y/constexpr-83116.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 0455be1d6da..518798e0c43 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1588,7 +1588,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
   tree result = NULL_TREE;
 
   constexpr_call *entry = NULL;
-  if (depth_ok && !non_constant_args)
+  if (depth_ok && !non_constant_args && ctx->strict)
     {
       new_call.hash = iterative_hash_template_arg
 	(new_call.bindings, constexpr_fundef_hasher::hash (new_call.fundef));
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
index e69de29bb2d..18d79e2e1cc 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
@@ -0,0 +1,18 @@
+// PR c++/83116
+// { dg-do run { target c++14 } }
+// { dg-options "-O2" }
+
+struct S {
+  constexpr S () : s(0) { foo (); }
+  constexpr int foo () { return s; }
+  int s;
+};
+
+int
+main ()
+{
+  static S var;
+  var.s = 5;
+  if (var.s != 5 || var.foo () != 5)
+    __builtin_abort ();
+}

	Marek
Jason Merrill Dec. 18, 2017, 9:13 p.m. | #3
OK.

On Mon, Dec 18, 2017 at 2:56 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Mon, Dec 18, 2017 at 01:07:18PM -0500, Jason Merrill wrote:

>> On Mon, Dec 18, 2017 at 10:09 AM, Marek Polacek <polacek@redhat.com> wrote:

>> > Here the problem was that cxx_eval_call_expression can cache the result of a

>> > constexpr call in constexpr_call_table, but we have to be careful, after

>> > store_init_value the result might be invalid.  So I believe we also have to

>> > clear the constexpr call table.  I've lumped it together with clearing

>> > cv_cache.

>>

>> Hmm, that seems like a big hammer; the problem isn't that

>> store_init_value makes the result invalid, it's that the result

>> calculated during store_init_value (when we can treat the object as

>> constant) isn't relevant later (when the object is no longer

>> constant).  So we want to avoid caching when we're called for the

>> initial value.  Maybe by changing

>>

>> if (depth_ok && !non_constant_args)

>>

>> to

>>

>> if (depth_ok && !non_constant_args && ctx->strict)

>>

>> ?  Does that work?

>

> Yes!  I wish I'd thought of ctx->strict before.  Well, something to remember.

>

> Bootstrapped/regtested on x86_64-linux, ok for trunk/7?

>

> 2017-12-18  Marek Polacek  <polacek@redhat.com>

>

>         PR c++/83116

>         * constexpr.c (cxx_eval_call_expression): Only look into

>         constexpr_call_table if ctx->strict.

>

>         * g++.dg/cpp1y/constexpr-83116.C: New test.

>

> diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c

> index 0455be1d6da..518798e0c43 100644

> --- gcc/cp/constexpr.c

> +++ gcc/cp/constexpr.c

> @@ -1588,7 +1588,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,

>    tree result = NULL_TREE;

>

>    constexpr_call *entry = NULL;

> -  if (depth_ok && !non_constant_args)

> +  if (depth_ok && !non_constant_args && ctx->strict)

>      {

>        new_call.hash = iterative_hash_template_arg

>         (new_call.bindings, constexpr_fundef_hasher::hash (new_call.fundef));

> diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C

> index e69de29bb2d..18d79e2e1cc 100644

> --- gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C

> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C

> @@ -0,0 +1,18 @@

> +// PR c++/83116

> +// { dg-do run { target c++14 } }

> +// { dg-options "-O2" }

> +

> +struct S {

> +  constexpr S () : s(0) { foo (); }

> +  constexpr int foo () { return s; }

> +  int s;

> +};

> +

> +int

> +main ()

> +{

> +  static S var;

> +  var.s = 5;

> +  if (var.s != 5 || var.foo () != 5)

> +    __builtin_abort ();

> +}

>

>         Marek

Patch

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 0455be1d6da..e0299e731f0 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -4992,21 +4992,23 @@  maybe_constant_value (tree t, tree decl)
   return r;
 }
 
-/* Dispose of the whole CV_CACHE.  */
+/* Dispose of the whole CV_CACHE and CONSTEXPR_CALL_TABLE.  */
 
 static void
-clear_cv_cache (void)
+clear_constexpr_cache (void)
 {
   if (cv_cache != NULL)
     cv_cache->empty ();
+  if (constexpr_call_table != NULL)
+    constexpr_call_table->empty ();
 }
 
-/* Dispose of the whole CV_CACHE and FOLD_CACHE.  */
+/* Clear the constexpr caches and FOLD_CACHE.  */
 
 void
-clear_cv_and_fold_caches (void)
+clear_constexpr_and_fold_caches (void)
 {
-  clear_cv_cache ();
+  clear_constexpr_cache ();
   clear_fold_cache ();
 }
 
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index f5f974da728..6e553c4f62d 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -7409,7 +7409,7 @@  extern bool var_in_maybe_constexpr_fn           (tree);
 extern void explain_invalid_constexpr_fn        (tree);
 extern vec<tree> cx_error_context               (void);
 extern tree fold_sizeof_expr			(tree);
-extern void clear_cv_and_fold_caches		(void);
+extern void clear_constexpr_and_fold_caches	(void);
 
 /* In cp-ubsan.c */
 extern void cp_ubsan_maybe_instrument_member_call (tree);
diff --git gcc/cp/decl.c gcc/cp/decl.c
index ca9e0c7b205..bed2280b9e4 100644
--- gcc/cp/decl.c
+++ gcc/cp/decl.c
@@ -14341,7 +14341,7 @@  finish_enum_value_list (tree enumtype)
 
   /* Each enumerator now has the type of its enumeration.  Clear the cache
      so that this change in types doesn't confuse us later on.  */
-  clear_cv_and_fold_caches ();
+  clear_constexpr_and_fold_caches ();
 }
 
 /* Finishes the enum type. This is called only the first time an
diff --git gcc/cp/typeck2.c gcc/cp/typeck2.c
index e5bb249b2be..787e59b70d8 100644
--- gcc/cp/typeck2.c
+++ gcc/cp/typeck2.c
@@ -845,7 +845,7 @@  store_init_value (tree decl, tree init, vec<tree, va_gc>** cleanups, int flags)
   value = replace_placeholders (value, decl);
 
   /* DECL may change value; purge caches.  */
-  clear_cv_and_fold_caches ();
+  clear_constexpr_and_fold_caches ();
 
   /* If the initializer is not a constant, fill in DECL_INITIAL with
      the bits that are constant, and then return an expression that
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
index e69de29bb2d..18d79e2e1cc 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-83116.C
@@ -0,0 +1,18 @@ 
+// PR c++/83116
+// { dg-do run { target c++14 } }
+// { dg-options "-O2" }
+
+struct S {
+  constexpr S () : s(0) { foo (); }
+  constexpr int foo () { return s; }
+  int s;
+};
+
+int
+main ()
+{
+  static S var;
+  var.s = 5;
+  if (var.s != 5 || var.foo () != 5)
+    __builtin_abort ();
+}