On Mon, Jun 30, 2025 at 04:56:47PM -0400, Jason Merrill wrote:
> On 6/30/25 4:24 PM, Jakub Jelinek wrote:
> > On Mon, Jun 30, 2025 at 03:55:46PM -0400, Jason Merrill wrote:
> > > Might go with my earlier !TREE_SIDE_EFFECTS && tree_invariant_p 
> > > suggestion?
> > 
> > If you mean the following, it passes the 2 testcases and I can surely
> > bootstrap/regtest it tonight.
> 
> Yes, that's what I meant, but did you also see my other two comments in the
> body of the patch?

Sorry for missing those.

Here is an updated patch with address_invariant_p function and doing
ARRAY_TYPE and POINTER_TYPE_P checks just once on TREE_TYPE (array).
Compared to what tree_invariant_p_1 was doing before, had to add
STRIP_ANY_LOCATION_WRAPPER (t); before the return line.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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.

--- gcc/tree.h.jj       2025-06-27 19:00:14.288493687 +0200
+++ gcc/tree.h  2025-06-30 23:05:01.647732780 +0200
@@ -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);
--- gcc/tree.cc.jj      2025-06-27 19:00:14.287493699 +0200
+++ gcc/tree.cc 2025-06-30 23:07:46.499662441 +0200
@@ -4015,6 +4015,38 @@ decl_address_ip_invariant_p (const_tree
   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
 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;
--- gcc/cp/typeck.cc.jj 2025-06-30 22:30:59.513417507 +0200
+++ gcc/cp/typeck.cc    2025-06-30 23:13:27.497379696 +0200
@@ -4001,13 +4001,61 @@ cp_build_array_ref (location_t loc, tree
       }
 
     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;
 
--- gcc/testsuite/g++.dg/ubsan/pr120471.C.jj    2025-06-30 22:31:15.428216062 
+0200
+++ gcc/testsuite/g++.dg/ubsan/pr120471.C       2025-06-30 22:31:15.428216062 
+0200
@@ -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);
+}
--- gcc/testsuite/g++.dg/parse/pr120471.C.jj    2025-06-30 22:31:15.428216062 
+0200
+++ gcc/testsuite/g++.dg/parse/pr120471.C       2025-06-30 22:31:15.428216062 
+0200
@@ -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)];
+}


        Jakub

Reply via email to