Martin Sebor <mse...@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
