On Wed, 29 Oct 2025, Christopher Bazley wrote:
> I don't think this fix is sufficient. It may appear to "fix" a specific crash,
> but only because it hides the symptom. I considered making this one-line
> change but when I discovered how broken the error-handling
> of vect_build_slp_instance is, I rejected the idea.
>
> Your proposed fix relies on an undocumented contract that
> vect_build_slp_instance always takes ownership of scalar_stmts, but the
> function does not honour that contract. I think the fact that this code was
> written wrongly in the first place and appears irregular in your proposed fix
> shows that this contract (even if it were documented) is not conducive to
> correct code.
This is the contract. It might be your interpretation that the function
is written wrongly, but I don't think that it is.
> If vect_build_slp_instance fails after successfully invoking
> vect_build_slp_tree, then scalar_stmts is owned jointly by vec_info and
> bst_map (via the tree node, because of SLP_TREE_SCALAR_STMTS (res) = stmts and
> SLP_TREE_REF_COUNT (res)++ in vect_build_slp_tree). Before returning false to
> the calling code that you propose to modify, vect_build_slp_instance invokes
> vect_free_slp_tree (which should be a no-op other than the fact that it
> reduces the reference count of the tree node) and may or may not also call
> scalar_stmts.release (which would free the vector but leave bst_map with a
> reference to the tree node that has a pointer to the same vector).
Yes, that's intended and correct behavior.
>
> Your proposed fix prevents a double-free of scalar_stmts upon destruction of
> the bb_vinfo (which releases the vector via SLP_TREE_SCALAR_STMTS
> (this).release () in ~_slp_tree) but it does not prevent
> release_scalar_stmts_to_slp_tree_map (bst_map) from doing another double-free
> on exit from vect_analyze_slp.
Why would that perform a double-free? SLP nodes are reference counted,
if entered into bst_map that increments the count, vect_build_slp_tree
gets you a result with one "reference" to your disposal (so the
vect_free_slp_node on error is correct - we hand back that reference).
> Conversely, if vect_build_slp_instance exits early (e.g. *limit == 0) without
> freeing scalar_stmts, your proposed change introduces a new leak by clearing
> the caller’s pointer to the vector without releasing it.
True, that early exit misses the release, I'll amend the fix with that.
> Please re-read '[RFC 6/9] Fix vexed ownership of stmts passed to
> vect_build_slp_instance' for further details, and also consider the other
> calling code simplified by that patch, which is not simplified by your
> proposed fix.
As said the fix is incorrect.
Unfortunately the patch does not give a way to reproduce the observed
failure.
Richard.
> On 29/10/2025 09:04, Richard Biener wrote:
> > vect_build_slp_instance always releases the scalar stmts vector, so make
> > sure
> > to mark it as released.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu. This should fix
> > the observed double-free?
> >
> > Thanks,
> > Richard.
> >
> > * tree-vect-slp.cc (vect_analyze_slp): Mark stmts in BB roots
> > as released after vect_build_slp_instance.
> > ---
> > gcc/tree-vect-slp.cc | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> > index e02b3379bb4..1ea2b066bfa 100644
> > --- a/gcc/tree-vect-slp.cc
> > +++ b/gcc/tree-vect-slp.cc
> > @@ -5564,10 +5564,10 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> > max_tree_size,
> > bb_vinfo->roots[i].remain,
> > max_tree_size, &limit, bst_map, false))
> > {
> > - bb_vinfo->roots[i].stmts = vNULL;
> > bb_vinfo->roots[i].roots = vNULL;
> > bb_vinfo->roots[i].remain = vNULL;
> > }
> > + bb_vinfo->roots[i].stmts = vNULL;
> > }
> > }
> >
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
