[PATCH] Make strlen range computations more conservative

[Jeff Law]

On 08/21/2018 10:15 PM, Bernd Edlinger wrote:
> On 08/22/18 00:43, Jeff Law wrote:
>> [ I'm still digesting, but saw something in this that ought to be broken
>> out... ]
>>
>> On 08/19/2018 09:55 AM, Bernd Edlinger wrote:
>>> diff -Npur gcc/tree-ssa-dse.c gcc/tree-ssa-dse.c
>>> --- gcc/tree-ssa-dse.c	2018-07-18 21:21:34.000000000 +0200
>>> +++ gcc/tree-ssa-dse.c	2018-08-19 14:29:32.344498771 +0200
>>> @@ -248,6 +248,12 @@ compute_trims (ao_ref *ref, sbitmap live
>>>   	 residual handling in mem* and str* functions is usually
>>>   	 reasonably efficient.  */
>>>         *trim_tail = last_orig - last_live;
>>> +      /* Don't fold away an out of bounds access, as this defeats proper
>>> +	 warnings.  */
>>> +      if (*trim_tail
>>> +	  && compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (ref->base)),
>>> +			       last_orig) <= 0)
>>> +	*trim_tail = 0;
>>>       }
>>>     else
>>>       *trim_tail = 0;
>> This seems like a good change in and of itself and should be able to go
>> forward without further review work.   Consider this hunk approved,
>> along with any testsuite you have which tickles this code (I didn't
>> immediately see one attached to this patch.  But I could have missed it).
>>
> 
> Sorry, for not being clear on this.
> 
> I needed both hunks "Don't fold away an out of bounds access, as this defeats proper
> warnings" to prevent a regression on gcc.dg/Wstringop-overflow-5.c,
> and surprise surprise, the xfail in gcc.dg/Wstringop-overflow-6.c suddenly popped up.
> 
> So without the unsafe range info, gcc.dg/Wstringop-overflow-5.c needs both hunks
> to not regress, but gcc.dg/Wstringop-overflow-6.c only needs the other one I committed
> yesterday.
> 
> So unfortunately I have no test case except gcc.dg/Wstringop-overflow-5.c for that.
> 
> 
> Still OK?
I almost had a WTF moment mis-parsing Wstringop-overflow-5 thinking it
had no dead stores.  But it's full of dead stores :-)

It sounds like the testsuite will tickle it when the right set of
patches are applied.  THere is no distinct test right now.

I went ahead and bootstrapped/regression tested the DSE change alone and
it doesn't trigger any regressions.  I'm going to install it -- I think
consensus has formed around not folding an out of bounds access so that
we can detect the problem and issue a suitable warning.  One less thing
to keep track of :-)


jeff