diff mbox series

[2/2] tree-eh: Fix up lhs_could_trap_p for *this [PR127133]

Message ID 20260901235731.2884382-2-andrew.pinski@oss.qualcomm.com
State New
Headers show
Series [1/2] lim/cselim: Use lhs_could_trap_p directly instead of inline | expand

Commit Message

Andrea Pinski Sept. 1, 2026, 11:57 p.m. UTC
Currently we were treating `this->a` as being non-trapping.
This is ok for the rhs but on the lhs it depends on the memory
was read only or not which we don't have access to right away.
So right now lhs_could_trap_p would call tree_could_trap_p
and then have some special cases for some read only/trapping
cases.  Instead let's split up tree_could_trap_p and then
have the special cases directly in the new function.

Bootstrapped and tested on x86_64-linux-gnu.

	PR tree-optimization/127133
	PR tree-optimization/127134

gcc/ChangeLog:

	* tree-eh.cc (tree_could_trap_p): Split into ...
	(tree_could_trap_1): This. Take into account lhs
	in some cases.
	(lhs_could_trap_p): Just call tree_could_trap_1.

gcc/testsuite/ChangeLog:

	* g++.dg/torture/const-method-this-1.C: New test.
	* g++.dg/torture/const-method-this-2.C: New test.

Signed-off-by: Andrea Pinski <andrew.pinski@oss.qualcomm.com>
---
 .../g++.dg/torture/const-method-this-1.C      | 26 +++++++++
 .../g++.dg/torture/const-method-this-2.C      | 27 +++++++++
 gcc/tree-eh.cc                                | 57 +++++++++++++------
 3 files changed, 92 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/const-method-this-1.C
 create mode 100644 gcc/testsuite/g++.dg/torture/const-method-this-2.C

Comments

Jeff Law Sept. 2, 2026, 1:28 a.m. UTC | #1
On 9/1/26 5:57 PM, Andrea Pinski wrote:
> Currently we were treating `this->a` as being non-trapping.
> This is ok for the rhs but on the lhs it depends on the memory
> was read only or not which we don't have access to right away.
> So right now lhs_could_trap_p would call tree_could_trap_p
> and then have some special cases for some read only/trapping
> cases.  Instead let's split up tree_could_trap_p and then
> have the special cases directly in the new function.
>
> Bootstrapped and tested on x86_64-linux-gnu.
>
> 	PR tree-optimization/127133
> 	PR tree-optimization/127134
>
> gcc/ChangeLog:
>
> 	* tree-eh.cc (tree_could_trap_p): Split into ...
> 	(tree_could_trap_1): This. Take into account lhs
> 	in some cases.
> 	(lhs_could_trap_p): Just call tree_could_trap_1.
>
> gcc/testsuite/ChangeLog:
>
> 	* g++.dg/torture/const-method-this-1.C: New test.
> 	* g++.dg/torture/const-method-this-2.C: New test.
>
> Signed-off-by: Andrea Pinski <andrew.pinski@oss.qualcomm.com>
OK
jeff
Andrea Pinski Sept. 2, 2026, 2:55 a.m. UTC | #2
On Tue, Sep 1, 2026 at 6:28 PM Jeff Law <jeffrey.law@oss.qualcomm.com> wrote:
>
>
>
> On 9/1/26 5:57 PM, Andrea Pinski wrote:
> > Currently we were treating `this->a` as being non-trapping.
> > This is ok for the rhs but on the lhs it depends on the memory
> > was read only or not which we don't have access to right away.
> > So right now lhs_could_trap_p would call tree_could_trap_p
> > and then have some special cases for some read only/trapping
> > cases.  Instead let's split up tree_could_trap_p and then
> > have the special cases directly in the new function.
> >
> > Bootstrapped and tested on x86_64-linux-gnu.
> >
> >       PR tree-optimization/127133
> >       PR tree-optimization/127134
> >
> > gcc/ChangeLog:
> >
> >       * tree-eh.cc (tree_could_trap_p): Split into ...
> >       (tree_could_trap_1): This. Take into account lhs
> >       in some cases.
> >       (lhs_could_trap_p): Just call tree_could_trap_1.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * g++.dg/torture/const-method-this-1.C: New test.
> >       * g++.dg/torture/const-method-this-2.C: New test.
> >
> > Signed-off-by: Andrea Pinski <andrew.pinski@oss.qualcomm.com>
> OK

Pushed with a small addition to both testcases of:
// { dg-additional-options "-fallow-store-data-races" }
To both of them.  I forgot to include them in the patch but I tested
with that there.
This was needed to invoke the bug in the first place.

> jeff
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/torture/const-method-this-1.C b/gcc/testsuite/g++.dg/torture/const-method-this-1.C
new file mode 100644
index 00000000000..d1142ee5e95
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/const-method-this-1.C
@@ -0,0 +1,26 @@ 
+// PR tree-optimization/127133
+// { dg-do run }
+
+struct s1
+{
+  void f(int b, int d) const;
+  int t;
+};
+
+__attribute__((noipa))
+void
+s1::f(int b, int d) const
+{
+  s1 &c = const_cast<s1&>(*this);
+  int p = t;
+  if (b)
+    c.t = d | p;
+}
+
+
+const s1 a = {1};
+
+int main()
+{
+  a.f(0,0);
+}
diff --git a/gcc/testsuite/g++.dg/torture/const-method-this-2.C b/gcc/testsuite/g++.dg/torture/const-method-this-2.C
new file mode 100644
index 00000000000..c77425b494f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/const-method-this-2.C
@@ -0,0 +1,27 @@ 
+// PR tree-optimization/127134
+// { dg-do run }
+
+struct s1
+{
+  void f(int b, int d) const;
+  int t;
+};
+
+__attribute__((noipa))
+void
+s1::f(int b, int d) const
+{
+  for(int i = 0; i < b; i++) {
+    s1 &c = const_cast<s1&>(*this);
+    int p = 1;
+    if (d)
+      c.t = p;
+  }
+}
+
+const s1 a = {1};
+
+int main()
+{
+  a.f(1,0);
+}
diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
index 0835f55af08..2d2d391f0a9 100644
--- a/gcc/tree-eh.cc
+++ b/gcc/tree-eh.cc
@@ -2754,10 +2754,11 @@  ref_outside_object_p (tree size, poly_offset_int off, tree refsz)
 
 /* Return true if EXPR can trap, as in dereferencing an invalid pointer
    location or floating point arithmetic.  C.f. the rtl version, may_trap_p.
-   This routine expects only GIMPLE lhs or rhs input.  */
+   This routine expects only GIMPLE lhs or rhs input.
+   LHS is true when LHS is a lhs.  */
 
-bool
-tree_could_trap_p (tree expr)
+static bool
+tree_could_trap_1 (tree expr, bool lhs)
 {
   enum tree_code code;
   bool fp_operation = false;
@@ -2813,7 +2814,7 @@  tree_could_trap_p (tree expr)
 
     case ARRAY_RANGE_REF:
       base = TREE_OPERAND (expr, 0);
-      if (tree_could_trap_p (base))
+      if (tree_could_trap_1 (base, lhs))
 	return true;
       if (TREE_THIS_NOTRAP (expr))
 	return false;
@@ -2821,7 +2822,7 @@  tree_could_trap_p (tree expr)
 
     case ARRAY_REF:
       base = TREE_OPERAND (expr, 0);
-      if (tree_could_trap_p (base))
+      if (tree_could_trap_1 (base, lhs))
 	return true;
       if (TREE_THIS_NOTRAP (expr))
 	return false;
@@ -2830,7 +2831,7 @@  tree_could_trap_p (tree expr)
     case TARGET_MEM_REF:
     case MEM_REF:
       if (TREE_CODE (TREE_OPERAND (expr, 0)) == ADDR_EXPR
-	  && tree_could_trap_p (TREE_OPERAND (TREE_OPERAND (expr, 0), 0)))
+	  && tree_could_trap_1 (TREE_OPERAND (TREE_OPERAND (expr, 0), 0), lhs))
 	return true;
       if (TREE_THIS_NOTRAP (expr))
 	return false;
@@ -2851,7 +2852,9 @@  tree_could_trap_p (tree expr)
 	  tree refsz = TYPE_SIZE_UNIT (TREE_TYPE (expr));
 	  return ref_outside_object_p (size, off, refsz);
 	}
-      if (cfun
+      /* See if the base is this for C++ methods. For LHS, this can still
+	 trap.  */
+      if (!lhs && cfun
 	  && TREE_CODE (TREE_TYPE (cfun->decl)) == METHOD_TYPE
 	  && ((TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
 	       && SSA_NAME_IS_DEFAULT_DEF (TREE_OPERAND (expr, 0))
@@ -2884,10 +2887,13 @@  tree_could_trap_p (tree expr)
       if (!t || !DECL_P (t))
 	return true;
       if (DECL_WEAK (t))
-	return tree_could_trap_p (t);
+	return tree_could_trap_1 (t, lhs);
       return false;
 
     case FUNCTION_DECL:
+      /* Functions will cause a trap if on the lhs.  */
+      if (lhs)
+	return true;
       /* Assume that accesses to weak functions may trap, unless we know
 	 they are certainly defined in current TU or in some other
 	 LTO partition.  */
@@ -2901,6 +2907,9 @@  tree_could_trap_p (tree expr)
       return false;
 
     case VAR_DECL:
+      /* Readonly non-local decls can cause a trap on the lhs.  */
+      if (lhs && !auto_var_p (expr) && TREE_READONLY (expr))
+	return true;
       /* Assume that accesses to weak vars may trap, unless we know
 	 they are certainly defined in current TU or in some other
 	 LTO partition.  */
@@ -2912,25 +2921,37 @@  tree_could_trap_p (tree expr)
 	  return !(node && node->in_other_partition);
 	}
       return false;
-
+    /* Strings, const and labels will cause a trap if on the lhs.  */
+    case LABEL_DECL:
+    case CONST_DECL:
+    case STRING_CST:
+      return lhs;
+    /* Result and arguments will never cause a trap.  */
+    case RESULT_DECL:
+    case PARM_DECL:
+      return false;
     default:
       return false;
     }
 }
 
+
+
+/* Return true if EXPR can trap, as in dereferencing an invalid pointer
+   location or floating point arithmetic.  C.f. the rtl version, may_trap_p.
+   This routine expects only GIMPLE lhs or rhs input.  */
+
+bool
+tree_could_trap_p (tree expr)
+{
+  return tree_could_trap_1 (expr, false);
+}
+
 /* Returns true if LHS is known not to trap as a store.  */
 bool
 lhs_could_trap_p (tree lhs)
 {
-  tree lhsbase = get_base_address (lhs);
-  if (tree_could_trap_p (lhs))
-    return true;
-  /* tree_could_trap_p is a predicate for loads, so check
-     for readonly memory explicitly.  */
-  if ((DECL_P (lhsbase) && TREE_READONLY (lhsbase))
-      || TREE_CODE (lhsbase) == STRING_CST)
-    return true;
-  return false;
+  return tree_could_trap_1 (lhs, true);
 }
 
 /* Return non-NULL if there is an integer operation with trapping overflow