On Mon, Jan 01, 2018 at 03:31:46PM -0700, Martin Sebor wrote:
> 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.
This is wrong for many reasons.
The recorded range info is a property of the SSA_NAME, can't be dependent on
where it is used, you are changing it globally. You don't really know how
the minoff and maxoff of the previous range were computed, so blindly
decreasing it is wrong, you can decrease it that way multiple times, or from
bounds that were computed some other way. And you aren't checking what
cstoff is, it could be excessively large, or could make the range invalid
(larger minoff than maxoff), etc.
Just 4 random examples that will be miscompiled by your patch:
void
foo (char *dst, char *src, int x)
{
__SIZE_TYPE__ n = __builtin_strlen (dst);
if (x)
{
__builtin_strcat (dst, "1");
__builtin_strcat (dst, src);
}
else
{
__builtin_strcat (dst, "234567890");
__builtin_strcat (dst, src);
}
if (n >= __PTRDIFF_MAX__ - 4)
__builtin_abort ();
}
void
bar (char *dst, char *src, int x)
{
__SIZE_TYPE__ n = __builtin_strlen (dst);
if (n > 145)
__builtin_unreachable ();
if (x)
{
__builtin_strcat (dst, "1");
__builtin_strcat (dst, src);
}
else
{
__builtin_strcat (dst, "234567890");
__builtin_strcat (dst, src);
}
if (n >= 140)
__builtin_abort ();
}
void
baz (char *dst, char *src)
{
__SIZE_TYPE__ n = __builtin_strlen (dst);
if (n > __PTRDIFF_MAX__ - 10)
__builtin_abort ();
might_not_return ();
__builtin_strcat (dst, "234abcdefghijklmnopqrstuv");
__builtin_strcat (dst, src);
}
void
boo (char *dst, char *src, int x)
{
__SIZE_TYPE__ n = __builtin_strlen (dst);
if (n < __PTRDIFF_MAX__ - 10)
{
__builtin_strcat (dst, "234abcdefghijklmnopqrstuv");
__builtin_strcat (dst, src);
}
}
In foo, while the range of n comes from the restriction that strlen should
return < (size_t) PTRDIFF_MAX value, you are decreasing the maxoff not once,
but twice. In bar, additionally the range of n comes from the builtin
unreachable assertion, the user says that strlen return value will be 0 to
145 inclusive, trying to decrease it in any way doesn't make sense.
In baz, if might_not_return returns, then it is true that n will need to be
smaller than __PTRDIFF_MAX__ - 10, but if might_not_return doesn't return,
n might be in a valid program __PTRDIFF_MAX__ - 1.
And finally in boo, the strcat calls are conditional you are changing it
again globally.
Jakub
