On 6/29/25 2:19 PM, Jason Merrill wrote:
On 6/28/25 3:08 AM, Jakub Jelinek wrote:
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),
!TREE_SIDE_EFFECTS && tree_invariant_p?
Or perhaps introduce a predicate that checks whether it's safe to use an
expression in both branches of a COND_EXPR, i.e. if there are no
SAVE_EXPR/TARGET_EXPR/BIND_EXPR?
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.
And my comment in that thread was that pushing the indexing down into
the COND_EXPR is probably undesirable once we start doing "significant
optimization on trees" so maybe we should just drop it?
I see that simply removing the COND_EXPR case from cp_build_array_ref
brings back the ICE from that thread, so more work is needed.
I guess your suggestion of only pushing the indexing down for simple
indexes makes sense, otherwise do the *(a ? &b : &c)[d] transformation.
For release branches, would unshare_expr (idx) for the second arm be
enough?
Ah, no, that doesn't help with SAVE_EXPR.
Jason
