On Fri, Jun 27, 2025 at 06:49:12PM -0400, Jason Merrill wrote: > On 6/27/25 5:58 PM, Jakub Jelinek wrote: > > 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 using cp_save_expr and if needed, evaluating it before the > > condition; for constant indexes this shouldn't change anything, and for > > larger expressions in idx I think the patch should result in smaller > > generated code, no need to duplicate all the evaluation in each of the > > branches. > > Evaluating idx before op0 seems to violate > https://eel.is/c++draft/expr#cond-1 > > "The first expression is sequenced before the second or third expression > ([intro.execution])."
Oops, yes. That one could be still handled by turning the condition into SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0>. But for C++17 there is also "In a subscript expression E1[E2], E1 is sequenced before E2." and I'm not sure how can that be handled. So, kill the optimization unless idx is INTEGER_CST (am not sure TREE_SIDE_EFFECTS is sufficient because it could be e.g. reading from some VAR_DECL), and perhaps try to recover the optimization in GIMPLE? Though, I think we don't gimplify ARRAY_REF into ARRAY_REF if the first operand is not an array and don't create ARRAY_REFs for what has been written as *(arr + idx). On the other side, ARRAY_REF is beneficial primary for the simple cases, so maybe if op1 and op2 are ADDR_EXPRs of non-volatile VAR_DECLs or COMPONENT_REFs with VAR_DECL bases so constant or constant offset from frame, E1 is sequenced before E2 won't be really observable and we could use the 3 SAVE_EXPRs. I think it has been added for https://gcc.gnu.org/pipermail/gcc-patches/2001-April/thread.html#48160 but I'm sure GIMPLE in between FE and expansion changed things sufficiently that it won't ICE anymore. Jakub
