https://gcc.gnu.org/g:988e87b66882875b14a6cab11c17516863c74a63

commit r16-1893-g988e87b66882875b14a6cab11c17516863c74a63
Author: Jakub Jelinek <ja...@redhat.com>
Date:   Tue Jul 1 15:28:10 2025 +0200

    c++: Fix up cp_build_array_ref COND_EXPR handling [PR120471]
    
    The following testcase is miscompiled since the introduction of UBSan,
    cp_build_array_ref COND_EXPR handling replaces
    (cond ? a : b)[idx] with cond ? a[idx] : b[idx], but if there are
    SAVE_EXPRs inside of idx, they will be evaluated just in one of the
    branches and the other uses uninitialized temporaries.
    
    Fixed by keeping doing what it did if idx doesn't have side effects
    and is invariant.  Otherwise if op1/op2 are ARRAY_TYPE arrays with
    invariant addresses or pointers with invariant values, use
    SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0> as a new condition
    and SAVE_EXPR <idx> instead of idx for the recursive calls.
    Otherwise punt, but if op1/op2 are ARRAY_TYPE, furthermore call
    cp_default_conversion on array, so that COND_EXPR with ARRAY_TYPE doesn't
    survive in the IL until expansion.
    
    2025-07-01  Jakub Jelinek  <ja...@redhat.com>
    
            PR c++/120471
    gcc/
            * tree.h (address_invariant_p): New function.
            * tree.cc (address_invariant_p): New function.
            (tree_invariant_p_1): Use it for ADDR_EXPR handling.  Formatting
            tweak.
    gcc/cp/
            * typeck.cc (cp_build_array_ref) <case COND_EXPR>: If idx is not
            INTEGER_CST, don't optimize the case (but cp_default_conversion on
            array early if it has ARRAY_TYPE) or use
            SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0> as new op0 
depending
            on flag_strong_eval_order and whether op1 and op2 are arrays with
            invariant address or tree invariant pointers.  Formatting fixes.
    gcc/testsuite/
            * g++.dg/ubsan/pr120471.C: New test.
            * g++.dg/parse/pr120471.C: New test.

Diff:
---
 gcc/cp/typeck.cc                      | 62 +++++++++++++++++++++++++++++++----
 gcc/testsuite/g++.dg/parse/pr120471.C | 42 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/ubsan/pr120471.C | 21 ++++++++++++
 gcc/tree.cc                           | 62 +++++++++++++++++++----------------
 gcc/tree.h                            |  4 +++
 5 files changed, 156 insertions(+), 35 deletions(-)

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index f4b49b792e3b..447fe810633d 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -4001,13 +4001,61 @@ cp_build_array_ref (location_t loc, tree array, tree 
idx,
       }
 
     case COND_EXPR:
-      ret = build_conditional_expr
-              (loc, TREE_OPERAND (array, 0),
-              cp_build_array_ref (loc, TREE_OPERAND (array, 1), idx,
-                                  complain),
-              cp_build_array_ref (loc, TREE_OPERAND (array, 2), idx,
-                                  complain),
-              complain);
+      tree op0, op1, op2;
+      op0 = TREE_OPERAND (array, 0);
+      op1 = TREE_OPERAND (array, 1);
+      op2 = TREE_OPERAND (array, 1);
+      if (TREE_SIDE_EFFECTS (idx) || !tree_invariant_p (idx))
+       {
+         /* If idx could possibly have some SAVE_EXPRs, turning
+            (op0 ? op1 : op2)[idx] into
+            op0 ? op1[idx] : op2[idx] can lead into temporaries
+            initialized in one conditional path and uninitialized
+            uses of them in the other path.
+            And if idx is a really large expression, evaluating it
+            twice is also not optimal.
+            On the other side, op0 must be sequenced before evaluation
+            of op1 and op2 and for C++17 op0, op1 and op2 must be
+            sequenced before idx.
+            If idx is INTEGER_CST, we can just do the optimization
+            without any SAVE_EXPRs, if op1 and op2 are both ARRAY_TYPE
+            VAR_DECLs or COMPONENT_REFs thereof (so their address
+            is constant or relative to frame), optimize into
+            (SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0>)
+            ? op1[SAVE_EXPR <idx>] : op2[SAVE_EXPR <idx>]
+            Otherwise avoid this optimization.  */
+         if (flag_strong_eval_order == 2)
+           {
+             if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
+               {
+                 if (!address_invariant_p (op1) || !address_invariant_p (op2))
+                   {
+                     /* Force default conversion on array if
+                        we can't optimize this and array is ARRAY_TYPE
+                        COND_EXPR, we can't leave COND_EXPRs with
+                        ARRAY_TYPE in the IL.  */
+                     array = cp_default_conversion (array, complain);
+                     if (error_operand_p (array))
+                       return error_mark_node;
+                     break;
+                   }
+               }
+             else if (!POINTER_TYPE_P (TREE_TYPE (array))
+                      || !tree_invariant_p (op1)
+                      || !tree_invariant_p (op2))
+               break;
+           }
+         if (TREE_SIDE_EFFECTS (idx))
+           {
+             idx = save_expr (idx);
+             op0 = save_expr (op0);
+             tree tem = build_compound_expr (loc, op0, idx);
+             op0 = build_compound_expr (loc, tem, op0);
+           }
+       }
+      op1 = cp_build_array_ref (loc, op1, idx, complain);
+      op2 = cp_build_array_ref (loc, op2, idx, complain);
+      ret = build_conditional_expr (loc, op0, op1, op2, complain);
       protected_set_expr_location (ret, loc);
       return ret;
 
diff --git a/gcc/testsuite/g++.dg/parse/pr120471.C 
b/gcc/testsuite/g++.dg/parse/pr120471.C
new file mode 100644
index 000000000000..ad47e380404a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr120471.C
@@ -0,0 +1,42 @@
+// PR c++/120471
+// { dg-do compile }
+
+extern int a1[], a2[], a3[], a4[];
+
+int corge (int);
+
+int
+foo (int p)
+{
+  return (p ? a1 : a2)[1];
+}
+
+int
+bar (int p, int q)
+{
+  return (p ? a1 : a2)[q];
+}
+
+int
+garply (int p, int q)
+{
+  return (p ? a1 : a2)[corge (q)];
+}
+
+int
+baz (int p, int q)
+{
+  return (p ? q ? a1 : a2 : q ? a3 : a4)[1];
+}
+
+int
+qux (int p, int q, int r)
+{
+  return (p ? q ? a1 : a2 : q ? a3 : a4)[r];
+}
+
+int
+fred (int p, int q, int r)
+{
+  return (p ? q ? a1 : a2 : q ? a3 : a4)[corge (r)];
+}
diff --git a/gcc/testsuite/g++.dg/ubsan/pr120471.C 
b/gcc/testsuite/g++.dg/ubsan/pr120471.C
new file mode 100644
index 000000000000..31b781f9aa7f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/pr120471.C
@@ -0,0 +1,21 @@
+// PR c++/120471
+// { dg-do run }
+// { dg-options "-fsanitize=undefined" }
+
+volatile int b[1], a[1];
+
+void
+foo (int x)
+{
+  volatile int c = 21;
+  volatile int v = (x % 2 ? b : a)[c % 3];
+  if (v != 0)
+    __builtin_abort ();
+}
+
+int
+main ()
+{
+  foo (1);
+  foo (2);
+}
diff --git a/gcc/tree.cc b/gcc/tree.cc
index e9a83e4260b3..6a055c8c2d0c 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -4015,6 +4015,38 @@ decl_address_ip_invariant_p (const_tree op)
   return false;
 }
 
+/* Return true if T is an object with invariant address.  */
+
+bool
+address_invariant_p (tree t)
+{
+  while (handled_component_p (t))
+    {
+      switch (TREE_CODE (t))
+       {
+       case ARRAY_REF:
+       case ARRAY_RANGE_REF:
+         if (!tree_invariant_p (TREE_OPERAND (t, 1))
+             || TREE_OPERAND (t, 2) != NULL_TREE
+             || TREE_OPERAND (t, 3) != NULL_TREE)
+           return false;
+         break;
+
+       case COMPONENT_REF:
+         if (TREE_OPERAND (t, 2) != NULL_TREE)
+           return false;
+         break;
+
+       default:
+         break;
+       }
+      t = TREE_OPERAND (t, 0);
+    }
+
+  STRIP_ANY_LOCATION_WRAPPER (t);
+  return CONSTANT_CLASS_P (t) || decl_address_invariant_p (t);
+}
+
 
 /* Return true if T is function-invariant (internal function, does
    not handle arithmetic; that's handled in skip_simple_arithmetic and
@@ -4023,10 +4055,7 @@ decl_address_ip_invariant_p (const_tree op)
 static bool
 tree_invariant_p_1 (tree t)
 {
-  tree op;
-
-  if (TREE_CONSTANT (t)
-      || (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)))
+  if (TREE_CONSTANT (t) || (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t)))
     return true;
 
   switch (TREE_CODE (t))
@@ -4036,30 +4065,7 @@ tree_invariant_p_1 (tree t)
       return true;
 
     case ADDR_EXPR:
-      op = TREE_OPERAND (t, 0);
-      while (handled_component_p (op))
-       {
-         switch (TREE_CODE (op))
-           {
-           case ARRAY_REF:
-           case ARRAY_RANGE_REF:
-             if (!tree_invariant_p (TREE_OPERAND (op, 1))
-                 || TREE_OPERAND (op, 2) != NULL_TREE
-                 || TREE_OPERAND (op, 3) != NULL_TREE)
-               return false;
-             break;
-
-           case COMPONENT_REF:
-             if (TREE_OPERAND (op, 2) != NULL_TREE)
-               return false;
-             break;
-
-           default:;
-           }
-         op = TREE_OPERAND (op, 0);
-       }
-
-      return CONSTANT_CLASS_P (op) || decl_address_invariant_p (op);
+      return address_invariant_p (TREE_OPERAND (t, 0));
 
     default:
       break;
diff --git a/gcc/tree.h b/gcc/tree.h
index 5de8d7e1ca17..e87fa0f81bc4 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5347,6 +5347,10 @@ extern tree staticp (tree);
 
 extern tree save_expr (tree);
 
+/* Return true if T is an object with invariant address.  */
+
+extern bool address_invariant_p (tree);
+
 /* Return true if T is function-invariant.  */
 
 extern bool tree_invariant_p (tree);

Reply via email to