Re: [PATCH] c++: 'mutable' subobject of constexpr variable [PR109745]

[Patrick Palka <ppalka@redhat.com>]

On Thu, 11 May 2023, Patrick Palka wrote:

> r13-2701-g7107ea6fb933f1 made us correctly accept 'mutable' member
> accesses during constexpr evaluation of objects constructed during that
> evaluation, while continuing to reject such accesses for constexpr
> objects constructed outside of that evaluation, by considering the
> CONSTRUCTOR_MUTABLE_POISON flag during cxx_eval_component_reference.
> 
> However, this flag is set only for the outermost CONSTRUCTOR of a
> constexpr variable initializer, so if we're accessing a 'mutable'
> subobject within a nested CONSTRUCTOR, the flag won't be set and
> we'll incorrectly accept the access.  This can lead to us rejecting
> valid code, as in the first testcase, or even wrong code due to

d'oh, this should say "this can lead to us accepting invalid code"

> speculative constexpr evaluation as in the second and third testcase.
> 
> This patch fixes this by setting CONSTRUCTOR_MUTABLE_POISON recursively
> rather than only on the outermost CONSTRUCTOR.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/13?
> 
> 	PR c++/109745
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck2.cc (poison_mutable_constructors): Define.
> 	(store_init_value): Use it instead of setting
> 	CONSTRUCTOR_MUTABLE_POISON directly.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/constexpr-mutable4.C: New test.
> 	* g++.dg/cpp0x/constexpr-mutable5.C: New test.
> 	* g++.dg/cpp1y/constexpr-mutable2.C: New test.
> ---
>  gcc/cp/typeck2.cc                             | 26 +++++++++++--
>  .../g++.dg/cpp0x/constexpr-mutable4.C         | 16 ++++++++
>  .../g++.dg/cpp0x/constexpr-mutable5.C         | 39 +++++++++++++++++++
>  .../g++.dg/cpp1y/constexpr-mutable2.C         | 20 ++++++++++
>  4 files changed, 97 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-mutable4.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-mutable5.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-mutable2.C
> 
> diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
> index f5cc7c8371c..8a187708482 100644
> --- a/gcc/cp/typeck2.cc
> +++ b/gcc/cp/typeck2.cc
> @@ -776,6 +776,27 @@ split_nonconstant_init (tree dest, tree init)
>    return code;
>  }
>  
> +/* T is the initializer of a constexpr variable.  Set CONSTRUCTOR_MUTABLE_POISON
> +   for any CONSTRUCTOR within T that contains (directly or indirectly) a mutable
> +   member, thereby poisoning it so it can't be copied to another a constexpr
> +   variable, or read during constexpr evaluation.  */
> +
> +static void
> +poison_mutable_constructors (tree t)
> +{
> +  if (TREE_CODE (t) != CONSTRUCTOR)
> +    return;
> +
> +  if (cp_has_mutable_p (TREE_TYPE (t)))
> +    {
> +      CONSTRUCTOR_MUTABLE_POISON (t) = true;
> +
> +      if (vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (t))
> +	for (const constructor_elt &ce : *elts)
> +	  poison_mutable_constructors (ce.value);
> +    }
> +}
> +
>  /* Perform appropriate conversions on the initial value of a variable,
>     store it in the declaration DECL,
>     and print any error messages that are appropriate.
> @@ -886,10 +907,7 @@ store_init_value (tree decl, tree init, vec<tree, va_gc>** cleanups, int flags)
>        else
>  	value = fold_non_dependent_init (value, tf_warning_or_error,
>  					 /*manifestly_const_eval=*/true, decl);
> -      if (TREE_CODE (value) == CONSTRUCTOR && cp_has_mutable_p (type))
> -	/* Poison this CONSTRUCTOR so it can't be copied to another
> -	   constexpr variable.  */
> -	CONSTRUCTOR_MUTABLE_POISON (value) = true;
> +      poison_mutable_constructors (value);
>        const_init = (reduced_constant_expression_p (value)
>  		    || error_operand_p (value));
>        DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = const_init;
> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable4.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable4.C
> new file mode 100644
> index 00000000000..01f32dea1bd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable4.C
> @@ -0,0 +1,16 @@
> +// PR c++/109745
> +// { dg-do compile { target c++11 } }
> +
> +struct A { mutable int m = 0; };
> +
> +struct B { A a; };
> +
> +struct C { B b; };
> +
> +int main() {
> +  constexpr B b;
> +  constexpr int bam = b.a.m;    // { dg-error "mutable" }
> +
> +  constexpr C c;
> +  constexpr int cbam = c.b.a.m; // { dg-error "mutable" }
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable5.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable5.C
> new file mode 100644
> index 00000000000..6a530e2abe6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-mutable5.C
> @@ -0,0 +1,39 @@
> +// PR c++/109745
> +// { dg-do run { target c++11 } }
> +// { dg-additional-options "-O" }
> +
> +struct A {
> +  mutable int m = 0;
> +  void f() const { ++m; };
> +  constexpr int get_m() const { return m; }
> +};
> +
> +struct B { A a; };
> +
> +struct C { B b; };
> +
> +int main() {
> +  constexpr A a;
> +  a.m++;
> +  if (a.get_m() != 1 || a.m != 1)
> +    __builtin_abort();
> +  a.m++;
> +  if (a.get_m() != 2 || a.m != 2)
> +    __builtin_abort();
> +
> +  constexpr B b;
> +  b.a.m++;
> +  if (b.a.get_m() != 1 || b.a.m != 1)
> +    __builtin_abort();
> +  b.a.m++;
> +  if (b.a.get_m() != 2 || b.a.m != 2)
> +    __builtin_abort();
> +
> +  constexpr C c;
> +  c.b.a.m++;
> +  if (c.b.a.get_m() != 1 || c.b.a.m != 1)
> +    __builtin_abort();
> +  c.b.a.m++;
> +  if (c.b.a.get_m() != 2 || c.b.a.m != 2)
> +    __builtin_abort();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-mutable2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-mutable2.C
> new file mode 100644
> index 00000000000..bb3429aea13
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-mutable2.C
> @@ -0,0 +1,20 @@
> +// PR c++/109745
> +// { dg-do run { target c++14 } }
> +// { dg-additional-options "-O" }
> +
> +template <typename T>
> +struct Foo { T val; };
> +
> +struct Bar {
> +  constexpr Bar() = default;
> +  constexpr Bar(Bar const& other) { other.val_ = 42; }
> +  constexpr int val() const { return val_; }
> +  mutable int val_{};
> +};
> +
> +int main() {
> +  constexpr Foo<Bar> x{};
> +  Foo<Bar> y{x};
> +  if (x.val.val() != 42 || x.val.val_ != 42)
> +    __builtin_abort();
> +}
> -- 
> 2.40.1.552.g91428f078b
> 
>