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

[Jason Merrill]

OK.

On Mon, Dec 18, 2017 at 2:56 PM, Marek Polacek  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  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  
>
> 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

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

[Marek Polacek]

On Mon, Dec 18, 2017 at 01:07:18PM -0500, Jason Merrill wrote:
> On Mon, Dec 18, 2017 at 10:09 AM, Marek Polacek  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  

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

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

[Jason Merrill]

On Mon, Dec 18, 2017 at 10:09 AM, Marek Polacek  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?