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