On 29/10/2025 13:04, Richard Biener wrote:
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.
A contract that is neither implemented, nor honoured by callers, nor
documented, is not a contract at all.
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.
The 'intended and correct behavior' leaves a dangling pointer (to the
stmts vector) in the created tree node.
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).
Because the tree node contains a pointer to the vector, which may or may
not have already been released.
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.
I do not think this is sufficient either. I considered all these ideas
before submitting what I believe to be a complete fix.
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.
I found it by testing BB SLP with predicated tails. You could presumably
do likewise.
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;
}
}
--
Christopher Bazley
Staff Software Engineer, GNU Tools Team.
Arm Ltd, 110 Fulbourn Road, Cambridge, CB1 9NJ, UK.
http://www.arm.com/
