Re: [PATCH] constrain strcat destination offset (PR 83642)

[Richard Sandiford <richard dot sandiford at linaro dot org>]

Martin Sebor <msebor@gmail.com> writes:
> The ICE in the test case submitted in PR tree-optimization/83640
> is triggered by the tree-ssa-strlen pass transforming calls to
> strcat to strcpy with an offset pointing to the terminating NUL
> of the destination string, and allowing the upper bound of the
> offset's range to exceed PTRDIFF_MAX by the length of
> the appended constant string.  Although the ICE itself is due
> to an unsafe assumption in the -Wrestrict checker, the excessive
> upper bound in the strcat case suggests an optimization opportunity
> that's missing from the tree-ssa-strlen pass: namely, to reduce
> the offset's upper bound by the length of the appended string.
> The attached patch adds this minor optimization.
>
> Martin
>
> PR tree-optimization/83642 - excessive strlen range after a strcat of non-empty string
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/83642
> 	* tree-ssa-strlen.c (handle_builtin_strcat): Set offset range.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/83642
> 	* gcc.dg/strlenopt-39.c: New test.
>
> diff --git a/gcc/testsuite/gcc.dg/strlenopt-39.c b/gcc/testsuite/gcc.dg/strlenopt-39.c
> new file mode 100644
> index 0000000..b070b6b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/strlenopt-39.c
> @@ -0,0 +1,84 @@
> +/* PR tree-optimization/83642] excessive strlen range after a strcat
> +   of non-empty string
> +   { dg-do compile }
> +   { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +void test_strcat_0_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_strcat (dst, "");
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__)
> +    __builtin_abort ();
> +}
> +
> +void test_strcat_1_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_strcat (dst, "1");
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 1)
> +    __builtin_abort ();
> +}
> +
> +void test_strcat_2_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_strcat (dst, "12");
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 2)
> +    __builtin_abort ();
> +}
> +
> +void test_strcat_3_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +  char a[] = "123";
> +
> +  __builtin_strcat (dst, a);
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 3)
> +    __builtin_abort ();
> +}
> +
> +void test_stpcpy_7_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_stpcpy (dst + n, "1234567");
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 7)
> +    __builtin_abort ();
> +}
> +
> +void test_strcpy_8_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_strcpy (dst + n, "12345678");
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 8)
> +    __builtin_abort ();
> +}
> +
> +void test_memcpy_9_strlen_range (char *dst, char *src)
> +{
> +  __SIZE_TYPE__ n = __builtin_strlen (dst);
> +
> +  __builtin_memcpy (dst + n, "123456789", 10);
> +  __builtin_strcat (dst, src);
> +
> +  if (n >= __PTRDIFF_MAX__ - 9)
> +    __builtin_abort ();
> +}
> +
> +/* { dg-final { scan-tree-dump-not "abort \\(\\)" "optimized" } } */
> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
> index be6ab9f..9e42232 100644
> --- a/gcc/tree-ssa-strlen.c
> +++ b/gcc/tree-ssa-strlen.c
> @@ -2486,10 +2486,35 @@ handle_builtin_strcat (enum built_in_function bcode, gimple_stmt_iterator *gsi)
>    if (endptr)
>      dst = fold_convert_loc (loc, TREE_TYPE (dst), unshare_expr (endptr));
>    else
> -    dst = fold_build2_loc (loc, POINTER_PLUS_EXPR,
> -			   TREE_TYPE (dst), unshare_expr (dst),
> -			   fold_convert_loc (loc, sizetype,
> -					     unshare_expr (dstlen)));
> +    {
> +      if (TREE_CODE (dstlen) == PLUS_EXPR
> +	  && TREE_CODE (TREE_OPERAND (dstlen, 0)) == SSA_NAME
> +	  && TREE_CODE (TREE_OPERAND (dstlen, 1)) == INTEGER_CST)
> +	{
> +	  /* For a call to strcat (DST, SRC) where DST = D + OFFSET and
> +	     where OFFSET = VAROFF + CSTOFF, adjust the upper bound of
> +	     VAROFF's range by the constant CSTOFF so that the result
> +	     is still guaranteed to be less than PTRDIFF_MAX, the size
> +	     of the longest possible string.  */
> +	  tree varoff = TREE_OPERAND (dstlen, 0);
> +	  wide_int minoff, maxoff;
> +	  value_range_type rng = get_range_info (varoff, &minoff, &maxoff);
> +
> +	  if (rng == VR_RANGE)
> +	    {
> +	      tree cstoff = TREE_OPERAND (dstlen, 1);
> +
> +	      maxoff -= wi::to_wide (cstoff);
> +	      set_range_info (varoff, rng, minoff, maxoff);
> +	    }
> +	}

I'm probably missing the point, but it seems unlikely that we can
unconditionally subtract CSTOFF from whatever the current recorded
maximum of VAROFF happens to be.  Shouldn't it be a maximum of
the current maxoff and PTRDIFF_MAX-cstoff instead?

Thanks,
Richard