On Mon, 3 Nov 2025, Andrew Pinski wrote:
> On Mon, Nov 3, 2025 at 7:21 AM Richard Biener <[email protected]> wrote:
> >
> > The following fixes path isolation changing the immediate use list of
> > an SSA name that is currently iterated over via FOR_EACH_IMM_USE_STMT.
> > This happens when it duplicates a BB within this and creates/modifies
> > stmts that contain SSA uses of the name and calls update_stmt which
> > re-builds SSA operands, including removal of SSA uses and re-inserting
> > them. This is not safe as that might cause missed iterated uses but
> > more importantly could cause the 'next' use to be removed.
> >
> > For the case in question the fix is to collect interesting uses in
> > a vec and do the processing outside of the FOR_EACH_IMM_USE_STMT
> > iteration.
> >
> > * gimple-ssa-isolated-paths.cc (check_loadstore): Set
> > the volatile flag on the stmt manually.
> > (find_implicit_erroneous_behavior): Move code transform
> > outside of FOR_EACH_IMM_USE_STMT iteration.
> > ---
> > gcc/gimple-ssa-isolate-paths.cc | 26 ++++++++++++++++----------
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/gcc/gimple-ssa-isolate-paths.cc
> > b/gcc/gimple-ssa-isolate-paths.cc
> > index ca1daf1d168..ffa04d15052 100644
> > --- a/gcc/gimple-ssa-isolate-paths.cc
> > +++ b/gcc/gimple-ssa-isolate-paths.cc
> > @@ -56,7 +56,7 @@ check_loadstore (gimple *stmt, tree op, tree, void *data)
> > {
> > TREE_THIS_VOLATILE (op) = 1;
> > TREE_SIDE_EFFECTS (op) = 1;
> > - update_stmt (stmt);
> > + gimple_set_has_volatile_ops (stmt, true);
> > return true;
> > }
> > return false;
> > @@ -762,6 +762,7 @@ find_implicit_erroneous_behavior (void)
> >
> > /* We've got a NULL PHI argument. Now see if the
> > PHI's result is dereferenced within BB. */
> > + auto_vec <gimple *, 4> uses_in_bb;
> > FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
> > {
> > /* We only care about uses in BB. Catching cases in
> > @@ -774,18 +775,23 @@ find_implicit_erroneous_behavior (void)
> > ? gimple_location (use_stmt)
> > : phi_arg_loc;
> >
> > - if (stmt_uses_name_in_undefined_way (use_stmt, lhs, loc)
> > - && (duplicate || can_duplicate_block_p (bb)))
> > + if (stmt_uses_name_in_undefined_way (use_stmt, lhs, loc))
> > {
> > - duplicate = isolate_path (bb, duplicate, e,
> > - use_stmt, lhs, false);
> > -
> > - /* When we remove an incoming edge, we need to
> > - reprocess the Ith element. */
> > - next_i = i;
> > - cfg_altered = true;
> > + if (!can_duplicate_block_p (bb))
> > + break;
> > + uses_in_bb.safe_push (use_stmt);
>
> I think can_duplicate_block_p can be pulled further up. I think when
> you did r13-3352-g5ad3cc1ecc3c7c you put it closest to the point of
> duplication rather than earliest as possible.
The function can be cleaned up further - I think we want to only
find the earliest UB stmt in the block and duplicate on that.
Possibly the others are diagnosed, but I'd have to check.
Both 2/3 and 3/3 were sent more for illustrative purposes (and trying
to get the checking bootstrapping).
> Also it seems like handle_return_addr_local_phi_arg has the same issue
> as find_implicit_erroneous_behavior with respect to
> FOR_EACH_IMM_USE_STMT. Though maybe that one is hard to hit.
Indeed it's quite random. The question is really what we want to
use FOR_EACH_IMM_USE_STMT for - quite some places use it as
"safe" and do-not-visit-stmts-multiple-times iteration. But
we could nail it down further and disallow any update_stmt call
not on the current iterated stmt (and have a global var tracking that).
FOR_EACH_IMM_USE_STMT is also quadratic in the number of immediate
uses due to it's re-sorting. If we'd have a spare bit on stmts just
for a replacement we could do
FOR_EACH_IMM_USE_FAST (...)
if (!use stmt marked)
{
v.safe_push (use_stmt);
mark stmt;
}
for (use_stmt : v)
unmark stmt;
and then iterate over a temporary vector (which might of course
be large). This isn't a replacement for the
FOR_EACH_IMM_USE_STMT / FOR_EACH_IMM_USE_ON_STMT style of iterating,
but that's usually safe - apart from the fold_stmt issue with ranger,
but that could then be fixed with my first patch removing the sentinel.
A C++y for (.. : ) could hide most of the above. But IMO the
other iteration styles also need for ( : ) support to not require
inconsistent coding.
But it really boils down how to most reliably verify current uses,
because removing the sentinel does make it less "safe" in the
current unconstrained setting.
As for flags on gimple stmt I wonder if we want sth like
auto_{bb,edge}_flag and ditch PLF and visited, possibly
modified. There's 32 bits of padding after num_ops at the moment
(though num_ops belongs to with-ops or even only to VLA stmt kinds).
Richard.
> Thanks,
> Andrew
>
> > }
> > }
> > + for (gimple *use_stmt : uses_in_bb)
> > + {
> > + duplicate = isolate_path (bb, duplicate, e,
> > + use_stmt, lhs, false);
> > +
> > + /* When we remove an incoming edge, we need to
> > + reprocess the Ith element. */
> > + next_i = i;
> > + cfg_altered = true;
> > + }
> > }
> > }
> > }
> > --
> > 2.51.0
> >
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
