On Mon, Dec 18, 2023 at 11:42:52AM -0500, Siddhesh Poyarekar wrote:
> It is always safe to set the computed bit for dynamic object sizes at
> the end of collect_object_sizes_for because even in case of a dependency
> loop encountered in nested calls, we have an SSA temporary to actually
> finish the object size expression. The reexamine pass for dynamic
> object sizes is only for propagation of unknowns and gimplification of
> the size expressions, not for loop resolution as in the case of static
> object sizes.
>
> gcc/ChangeLog:
>
> PR tree-optimization/113012
> * gcc.dg/ubsan/pr113012.c: New test case.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/113012
> * tree-object-size.cc (compute_builtin_object_size): Expand
> comment for dynamic object sizes.
> (collect_object_sizes_for): Always set COMPUTED bitmap for
> dynamic object sizes.
You have the gcc/ChangeLog and gcc/testsuite/ChangeLog hunks swapped,
I think this wouldn't get through pre-commit hook.
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -1185,10 +1185,12 @@ compute_builtin_object_size (tree ptr, int
> object_size_type,
> osi.tos = NULL;
> }
>
> - /* First pass: walk UD chains, compute object sizes that
> - can be computed. osi.reexamine bitmap at the end will
> - contain what variables were found in dependency cycles
> - and therefore need to be reexamined. */
> + /* First pass: walk UD chains, compute object sizes that can be
> computed.
> + osi.reexamine bitmap at the end will contain what variables that need
This wording is weird.
Perhaps ... will contain versions of SSA_NAMEs that need
to be reexamined. ?
Because varno seems to be SSA_NAME_VERSION.
> @@ -1823,11 +1825,16 @@ collect_object_sizes_for (struct object_size_info
> *osi, tree var)
> gcc_unreachable ();
> }
>
> - if (! reexamine || object_sizes_unknown_p (object_size_type, varno))
> + /* Dynamic sizes use placeholder temps to return an answer, so it is always
> + * safe to set COMPUTED for them. */
We don't use this style of comments, please replace the * at the start of
second line with a space.
> + if ((object_size_type & OST_DYNAMIC)
> + || !reexamine || object_sizes_unknown_p (object_size_type, varno))
> {
> bitmap_set_bit (computed[object_size_type], varno);
> if (!(object_size_type & OST_DYNAMIC))
> bitmap_clear_bit (osi->reexamine, varno);
> + else if (reexamine)
> + bitmap_set_bit (osi->reexamine, varno);
> }
> else
> {
Otherwise LGTM, but please wait at least a few weeks before backporting to
13.
Jakub
