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.
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).
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.
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.
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.
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;
}
}
--
Christopher Bazley
Staff Software Engineer, GNU Tools Team.
Arm Ltd, 110 Fulbourn Road, Cambridge, CB1 9NJ, UK.
http://www.arm.com/
