Re: RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 03:20:04PM -0500, Jason Merrill wrote:
> Tested x86_64-pc-linux-gnu. Are the tree.c changes OK for trunk?

The tree.c changes are ok.  But I have nits and one bigger issue in
constexpr.c:

> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -2214,6 +2214,9 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree 
> t,
>vec **p = _ELTS (ctx->ctor);
>vec_alloc (*p, vec_safe_length (v));
>  
> +  bool constant_p = true;
> +  bool side_effects_p = false;
> +
>unsigned i; tree index, value;

I think vars of different types shouldn't be declared on the same line.
And perhaps the empty line in between two sets of declarations isn't needed.

>FOR_EACH_CONSTRUCTOR_ELT (v, i, index, value)
>  {
> @@ -2826,6 +2836,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, 
> tree t,
>  }
>type = TREE_TYPE (object);
>bool no_zero_init = true;
> +
> +  vec *ctors = make_tree_vector();

Missing space before (.

>while (!refs->is_empty())
>  {
>if (*valp == NULL_TREE)
> @@ -2837,6 +2849,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, 
> tree t,
>subobjects will also be zero-initialized.  */
>no_zero_init = CONSTRUCTOR_NO_IMPLICIT_ZERO (*valp);
>  
> +  vec_safe_push (ctors, *valp);
> +
>enum tree_code code = TREE_CODE (type);
>type = refs->pop();
>tree index = refs->pop();
> @@ -2889,14 +2903,35 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, 
> tree t,
>/* The hash table might have moved since the get earlier.  */
>valp = ctx->values->get (object);
>if (TREE_CODE (init) == CONSTRUCTOR)
> - /* An outer ctx->ctor might be pointing to *valp, so just replace
> -its contents.  */
> - CONSTRUCTOR_ELTS (*valp) = CONSTRUCTOR_ELTS (init);
> + {
> +   /* An outer ctx->ctor might be pointing to *valp, so replace
> +  its contents.  */
> +   CONSTRUCTOR_ELTS (*valp) = CONSTRUCTOR_ELTS (init);
> +   TREE_CONSTANT (*valp) = TREE_CONSTANT (init);
> +   TREE_SIDE_EFFECTS (*valp) = TREE_SIDE_EFFECTS (init);
> + }
>else
>   *valp = init;
>  }
>else
> -*valp = init;
> +{
> +  *valp = init;
> +
> +  /* Update TREE_CONSTANT and TREE_SIDE_EFFECTS on enclosing
> +  CONSTRUCTORs.  */
> +  unsigned i; tree elt;
> +  bool c = TREE_CONSTANT (init);
> +  bool s = TREE_SIDE_EFFECTS (init);
> +  if (!c || s)
> + FOR_EACH_VEC_SAFE_ELT (ctors, i, elt)
> +   {
> + if (!c)
> +   TREE_CONSTANT (elt) = false;
> + if (s)
> +   TREE_SIDE_EFFECTS (elt) = true;
> +   }
> +}
> +  release_tree_vector (ctors);
>  
>if (*non_constant_p)
>  return t;
> @@ -3579,9 +3614,16 @@ cxx_eval_constant_expression (const constexpr_ctx 
> *ctx, tree t,
>  
>  case CONSTRUCTOR:
>if (TREE_CONSTANT (t))
> - /* Don't re-process a constant CONSTRUCTOR, but do fold it to
> -VECTOR_CST if applicable.  */
> - return fold (t);
> + {
> +   /* Don't re-process a constant CONSTRUCTOR, but do fold it to
> +  VECTOR_CST if applicable.  */
> +   if (CHECKING_P)
> + verify_constructor_flags (t);
> +   else
> + recompute_constructor_flags (t);

But I don't understand this.  Either the flags are supposed to be already
correct here, then I'd expect to see
  if (CHECKING_P)
verify_constructor_flags (t);
only, or they are not guaranteed to be correct, and then I'd expect
unconditional
  recompute_constructor_flags (t).

> +   if (TREE_CONSTANT (t))
> + return fold (t);
> + }
>r = cxx_eval_bare_aggregate (ctx, t, lval,
>  non_constant_p, overflow_p);
>break;

Jakub

Re: RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)

[Jason Merrill]

On 01/26/2016 03:32 PM, Jakub Jelinek wrote:

>+ if (CHECKING_P)
>+   verify_constructor_flags (t);
>+ else
>+   recompute_constructor_flags (t);



But I don't understand this.  Either the flags are supposed to be already
correct here, then I'd expect to see
   if (CHECKING_P)
 verify_constructor_flags (t);
only, or they are not guaranteed to be correct, and then I'd expect
unconditional
   recompute_constructor_flags (t).



They are supposed to be correct, so when --enable-checking, we check for 
that.  The recompute is for better fault-tolerance in release compilers 
in case the patch doesn't catch everything.


Jason

Re: RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 03:46:50PM -0500, Jason Merrill wrote:
> On 01/26/2016 03:32 PM, Jakub Jelinek wrote:
> >>>+if (CHECKING_P)
> >>>+  verify_constructor_flags (t);
> >>>+else
> >>>+  recompute_constructor_flags (t);
> 
> >But I don't understand this.  Either the flags are supposed to be already
> >correct here, then I'd expect to see
> >   if (CHECKING_P)
> > verify_constructor_flags (t);
> >only, or they are not guaranteed to be correct, and then I'd expect
> >unconditional
> >   recompute_constructor_flags (t).
> >
> 
> They are supposed to be correct, so when --enable-checking, we check for
> that.  The recompute is for better fault-tolerance in release compilers in
> case the patch doesn't catch everything.

Ah, ok.  But please make sure to remove it after GCC 6 branches. 

Jakub