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

[Jakub Jelinek <jakub@redhat.com>]

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.
> 
> 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.
> 
> 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.

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;
+      /* 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;
+    }
+
   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