On 01/08/2018 08:05 PM, Martin Sebor wrote:
> On 01/08/2018 04:28 AM, Richard Biener wrote:
>> On Sat, Jan 6, 2018 at 11:04 PM, Martin Sebor <mse...@gmail.com> wrote:
>>> Bug 83671 - Fix for false positive reported by -Wstringop-overflow
>>> does not work at -O1, points out that the string length range
>>> optimization implemented as a solution for bug 83373 doesn't help
>>> at -O1. The root cause is that the fix was added to the strlen
>>> pass that doesn't run at -O1.
>>>
>>> The string length range computation doesn't depend on the strlen
>>> pass, and so the range can be set earlier, in gimple-fold, and
>>> its results made available even at -O1. The attached patch
>>> changes the gimple_fold_builtin_strlen() function to do that.
>>>
>>> While testing the change I came across a number of other simple
>>> strlen cases that currently aren't handled, some at -O1, others
>>> at all. I added code to handle some of the simplest of them
>>> and opened bugs to remind us/myself to get back to the rest in
>>> the future (pr83693 and pr83702). The significant enhancement
>>> is handling arrays of arrays with non-constant indices and
>>> pointers to such things, such as in:
>>>
>>> char a[2][7];
>>>
>>> void f (int i)
>>> {
>>> if (strlen (a[i]) > 6) // eliminated with the patch
>>> abort ();
>>> }
>>>
>>> Attached is a near-minimal patch to handle PR 83671.
>>
>> Please don't use set_range_info form insinde fold_stmt (), this is
>> IMHO a layering violation.
>>
>> Why not restrict -Wstrinop-overflow to -O2+?
>
> For simple cases, -Wstrinop-overflow works even without
> optimization. Given the notoriety of buffer overflows and
> because not all software is or can always be compiled with
> -O2 or even with -O1, I think it's important to keep it working.
If something can't be compiled with -O2, then it can't really be
compiled with -O1. In that kind of scenario someone is papering over a bug.
While I am sympathetic to the desire to provide as many and as accurate
of warnings at -O1 as -O2, we have to balance that against the cost,
both in terms of compile-time and in terms of maintenance if we end up
having to duplicate code. We have to evaluate each case individually.
>
> I initially did consider resolving PR 83671 as won't fix because
> at -O1 neither the strlen pass not VRP runs. But as I explained
> above, because the range doesn't depend on the strlen pass and
> can be determined and set anywhere, regardless of the optimization
> level, doing it outside the pass seems like a win-win. Exposing
> the range early improves downstream transformations before strlen
> runs or even when it doesn't, and as a bonus, the warning also
> disappears.
Outside is a lose if we have to duplicate the analysis or significant
chunks of code, or even small amounts of code if they are complex.
>
> But I don't understand the concern. The range is only set when
> the expression is an SSA_NAME, and setting in this function here
> seems no different than setting it in one if its callers (which
> may be, indirectly. in a pass like strlen). If you have
> a suggestion for a better place to set it I'd be happy to move
> it, I just don't see the problem.So identifying and setting the range is
> analysis. Folding the statement
is an optimization. Setting the range from inside a folding routine is
a layering violation in general. Though frankly we have not been
particularly good at separation of analysis from optimization.
But I suspect this is a truly trivial part of the patch, so let's just
drop it as controversial.
I'll try to look at the actual patch in detail tomorrow.
Jeff
