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