[PATCH] Adjust tree-ssa-strlen.c for irange API.

[Martin Sebor]

On 8/4/20 3:23 PM, Aldy Hernandez wrote:
> 
> 
> On 8/4/20 9:34 PM, Martin Sebor wrote:
>> On 8/4/20 5:33 AM, Aldy Hernandez via Gcc-patches wrote:
>>> This patch adapts the strlen pass to use the irange API.
>>>
>>> I wasn't able to remove the one annoying use of VR_ANTI_RANGE, because
>>> I'm not sure what to do.  Perhaps Martin can shed some light.  The
>>> current code has:
>>>
>>>    else if (rng == VR_ANTI_RANGE)
>>>     {
>>>       wide_int maxobjsize = wi::to_wide (TYPE_MAX_VALUE 
>>> (ptrdiff_type_node));
>>>       if (wi::ltu_p (cntrange[1], maxobjsize))
>>>         {
>>>           cntrange[0] = cntrange[1] + 1;
>>>           cntrange[1] = maxobjsize;
>>>
>>> Suppose we have ~[10,20], won't the above set cntrange[] to [21,MAX]? 
>>> Won't
>>> this ignore the 0..9 that is part of the range?  What should we do here?
>>
>> cntrange is the range of the strncpy (and strncat) bound.  It does
>> ignore the lower subrange but I think that's intentional because
>> the lower the bound the more likely the truncation, so it serves
>> to minimize false positives.
>>
>> I didn't see any tests fail with the anti-range block disabled but
>> with some effort I was able to come up with one:
>>
>>    char a[7];
>>
>>    void f (int n)
>>    {
>>      if (n > 3)
>>        n = 0;
>>
>>      strncpy (a, "12345678", n);   // -Wstringop-truncation
>>    }
>>
>> The warning disappears when the anti-range handling is removed so
>> unless that's causing headaches for the new API I think we want to
>> keep it (and add the test case :)
> 
> Hi Martin.
> 
> Thanks for taking the time to respond.
> 
> On the strlen1 dump I see that the 3rd argument to strncpy above is:
> 
>    long unsigned int ~[4, 18446744071562067967]
> 
> which is a fancy way of saying:
> 
>    long unsigned int [0,3][18446744071562067967,+INF]
> 
> The second sub-range is basically [INT_MIN,+INF] for the original int N, 
> which makes sense because N could be negative on the way in.
> 
> I don't understand the warning though:
> 
> a.c:8:5: warning: ‘__builtin_strncpy’ output truncated copying between 0 
> and 3 bytes from a
>   string of length 8 [-Wstringop-truncation]
>      8 |     __builtin_strncpy (a, "12345678", n);   // 
> -Wstringop-truncation
>        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The range of the bound to strncpy can certainly be [0,3], but it can 
> also be [1844...,+INF] which shouldn't warn.

strncpy(d, s, n) zeroes out the destination after it copies s, up
to n - strlen (s).  It's been a while and -Wstringop-truncation
certainly has its quirks (to put it nicely) but I think this one
is a feature.

> 
> In a world without anti-ranges, we'd see the 2 sub-ranges above.  How 
> would you suggest handling it?  We could nuke out the uppermost 
> sub-range, but what if the range is [0,3][10,20]?  Perhaps remove from 
> some arbitrary number on up?  Say...[0xf.....,+INF]?  This seems like a 
> hack, but perhaps is what's needed???

The second subrange in [0, 3][10, 20] is out of bounds for char[7]
and the first one truncates so either we issue -Wstringop-truncation
or if not, -Wstringop-overflow.

A better example might be [0, 3][5, 7] with "1234" as the source.
Only one of these ranges truncates so a warning would probably not
be called for.  Only warn when there's no range that doesn't lead
to truncation.

It will get more interesting if/when the length of the source string
is also in a discontiguous range.  My head is already starting to hurt.

Martin

> 
> It doesn't seem like the above source should warn.  Am I missing something?
> 
> Thanks.
> Aldy
>