Richard Sandiford <richard.sandif...@linaro.org> writes:
> 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?
er, of course I meant minimum :-)
