Re: [PATCH] tree-object-size: Always set computed bit for bdos [PR113012]

[Jakub Jelinek <jakub@redhat.com>]

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