Re: [PATCH] tree: Fix up save_expr [PR52339]

[Jason Merrill <jason@redhat.com>]

On 5/5/23 06:45, Jakub Jelinek wrote:
> On Fri, May 05, 2023 at 11:55:41AM +0200, Jakub Jelinek via Gcc-patches wrote:
>> Looking at the Ada cases (I admit I don't really understand why it isn't
>> vectorized, the IL is so different from the start because of the extra
>> SAVE_EXPRs that it is very hard to diff stuff), the case where save_expr
>> used to return the argument and no longer does are those
>> r.P_BOUNDS->LB0
>> etc. cases.  Now, I wondered if (pre-gimplification) we couldn't make an
>> exception and allow the base to be INDIRECT_REF or of a REFERENCE_TYPE
>> with the idea that references are really imutable and can't be changed
>> during its lifetime (after gimplification whether something is
>> REFERENCE_TYPE or POINTER_TYPE is lost), but that isn't what Ada is using.

And anyway, a reference can also refer to a non-const object.

>> So, another possibility would be to allow bases of TREE_READONLY (t) &&
>> !TREE_SIDE_EFFECTS (t) which are INDIRECT_REFs of tree_invariant_p_1
>> addresses.  That doesn't work either, in the r.P_BOUNDS->LB0 case
>> P_BOUNDS is a FIELD_DECL with POINTER_TYPE, LB0 is TREE_READONLY FIELD_DECL
>> and that COMPONENT_REF is  also TREE_READONLY, r is TREE_READONLY PARM_DECL,
>> but unforuntately the r.P_BOUNDS COMPONENT_REF isn't marked TREE_READONLY.

And an invariant pointer can point to a non-const object.

>> Thus, shall we treat as tree_invariant_p_1 also handled components which
>> are !TREE_SIDE_EFFECTS (t), but not TREE_READONLY and only their base
>> is TREE_READONLY?  Or do that only during the recursion?
> But doing that feels quite risky.  While the following version of
> the patch avoids the Ada regressions, the fact that we don't miscompile
> the pr52339-1.c testcase modified to have
> int
> foo (const struct S *const p, struct S *q)
> rather than
> int
> foo (const struct S *p, struct S *q)
> is only because the FE happens to add there some useless cast in between.
> While the pointer is invariant, I'm afraid nothing guarantees it goes out
> of scope in between multiple uses of the expression returned by save_expr.

Right.

> 2023-05-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/52339
> 	* tree.cc (tree_invariant_p_1): For TREE_READONLY (t) without
> 	side-effects, only return true if DECL_P (get_base_address (t)).
> 
> 	* g++.dg/opt/pr52339.C: New test.
> 	* gcc.c-torture/execute/pr52339-1.c: New test.
> 	* gcc.c-torture/execute/pr52339-2.c: New test.
> 
> --- gcc/tree.cc.jj	2023-05-01 09:59:46.686293833 +0200
> +++ gcc/tree.cc	2023-05-05 12:34:26.989523468 +0200
> @@ -3876,10 +3876,26 @@ tree_invariant_p_1 (tree t)
>   {
>     tree op;
>   
> -  if (TREE_CONSTANT (t)
> -      || (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)))
> +  if (TREE_CONSTANT (t))
>       return true;
>   
> +  if (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
> +    {
> +      /* Return true for const qualified vars, but for members or array
> +	 elements without side-effects return true only if the base
> +	 object is a decl.  If the base is e.g. a pointer dereference,
> +	 what the pointer points to could be deallocated or the pointer
> +	 could be changed.  See PR52339.  */
> +      tree base = get_base_address (t);
> +      if (DECL_P (base))
> +	return true;

So I think the above is correct.

> +      /* As an exception, allow pointer dereferences as long as the pointer
> +	 is invariant.  */
> +      if (TREE_CODE (base) == INDIRECT_REF
> +	  && tree_invariant_p_1 (get_base_address (TREE_OPERAND (base, 0))))
> +	return true;

And this is unsafe.

> +    }
> +
>     switch (TREE_CODE (t))
>       {
>       case SAVE_EXPR:
> --- gcc/testsuite/g++.dg/opt/pr52339.C.jj	2023-05-04 15:23:20.459935705 +0200
> +++ gcc/testsuite/g++.dg/opt/pr52339.C	2023-05-04 15:22:35.640578681 +0200
> @@ -0,0 +1,19 @@
> +// PR c++/52339
> +// { dg-do run { target c++11 } }
> +
> +
> +struct B;
> +struct A { B *b; };
> +struct B {
> +  A *a;
> +  B () : a(new A{this}) {}
> +  ~B () { delete a; }
> +};
> +
> +int
> +main ()
> +{
> +  B *b = new B;
> +  const A *a = b->a;
> +  delete a->b;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr52339-1.c.jj	2023-05-04 15:22:59.177241023 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr52339-1.c	2023-05-04 15:20:19.820527142 +0200
> @@ -0,0 +1,29 @@
> +/* PR c++/52339 */
> +
> +struct S { int a; };
> +
> +void
> +bar (int *p, struct S *q)
> +{
> +  __builtin_free (q);
> +}
> +
> +int
> +foo (const struct S *p, struct S *q)
> +{
> +  int b[p->a];
> +  bar (b, q);
> +  return sizeof (b);
> +}
> +
> +int
> +main ()
> +{
> +  struct S *p = __builtin_malloc (sizeof (struct S));
> +  if (!p)
> +    return 0;
> +  p->a = 42;
> +  if (foo (p, p) != 42 * sizeof (int))
> +    __builtin_abort ();
> +  return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr52339-2.c.jj	2022-11-21 10:04:00.210677046 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr52339-2.c	2023-05-04 19:34:08.581686806 +0200
> @@ -0,0 +1,20 @@
> +/* PR c++/52339 */
> +
> +struct S { int a; };
> +
> +int
> +foo (const struct S *p)
> +{
> +  int b[p->a];
> +  ++p;
> +  return sizeof (b);
> +}
> +
> +int
> +main ()
> +{
> +  struct S s[] = { { 42 }, { 43 } };
> +  if (foo (s) != 42 * sizeof (int))
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 
> 	Jakub
>