Please review writeup for fixing PR 78809 (inline strcmp for small constant strings)

[Martin Sebor]

On 11/17/2017 04:08 PM, Qing Zhao wrote:
>
>> On Nov 17, 2017, at 1:50 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Thu, Nov 16, 2017 at 06:14:35PM -0700, Jeff Law wrote:
>>>> However, this routine currently miss a very obvious case as the following:
>>>>
>>>> char s[100] = {'a','b','c','d’};
>>>>
>>>> __builtin_strcmp(s, "abc") != 0
>>>>
>>>> So, I have to change this routine to include such common case.
>>>>
>>>> do you think using this routine is good? or do you have other
>>>> suggestions (since I am still not very familiar with the internals of
>>>> GCC, might not find the best available one now…)
>>> The range information attached to an SSA_NAME is global data.  ie, it
>>> must hold at all locations where the object in question might be
>>> referenced.  This implies that it will sometimes (often?) be less
>>> precise than you might like.
>>>
>>> I am currently working towards an embeddable context sensitive range
>>> analyzer that in theory could be used within tree-ssa-strlen pass to
>>> give more precise range information.  I'm hoping to wrap that work up in
>>> the next day or so so that folks can use it in gcc-8.
>>
>> Well, one thing is a range of integral SSA_NAME at a certain point, the
>> other is the length of the C string pointed by a certain pointer (that is
>> something the strlen pass tracks), and another case is the content of that
>> string, which you'd presumably need to optimize the strcmp at compile time.
>> I think current strlen pass ought to find out that strlen (s) is 4 at that
>> point, if it doesn't, please file a PR with a testcase.
>
> for the safety checking purpose, when we try to convert
>
> __builtin_strcmp(s, "abc") != 0
>
> to
>
> __builtin_memcmp (s, “abc”, 4) != 0
>
> we have to make sure that the size of variable “s” is larger than “4”.

Presumably you mean "is at least 4?"

>
> if  “s” is declared as
>
> char s[100];
>
> currently,  the “get_range_strlen” cannot determine its maximum length is 100. (it just return UNKNOWN).
>
> so, I have to update “get_range_strlen” for such simple case.
>
> this does provide the information I want.  However, since the routine “get_range_strlen” is also used in other places,
> for example, in gimple-ssa-sprintf.c,  the implementation of the sprintf overflow warning uses the routine “get_range_strlen”
> to decide the string’s maximum size and buffer size.
>
> my change in “get_range_strlen” triggered some new warnings for  -Werror=format-overflow (from gimple-ssa-sprintf.c
> mentioned above) as following:
>
> qinzhao@gcc116:~/Bugs/warning$ cat t.c
> #include <stdio.h>
>
> void foo(const char *macro)
> {
>   char buf1[256], buf2[256];
>   sprintf (buf1, "%s=%s", macro, buf2);
>   return;
> }
>
> with my private GCC:
>
> qinzhao@gcc116:~/Bugs/warning$ /home/qinzhao/Install/latest/bin/gcc t.c -Werror=format-overflow -S
> t.c: In function ‘foo’:
> t.c:6:18: error: ‘sprintf’ may write a terminating nul past the end of the destination [-Werror=format-overflow=]
>    sprintf (buf1, "%s=%s", macro, buf2);
>                   ^~~~~~~
> t.c:6:3: note: ‘sprintf’ output 2 or more bytes (assuming 257) into a destination of size 256
>    sprintf (buf1, "%s=%s", macro, buf2);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors

When the length of one or more of the strings referenced by
the argument passed to get_range_strlen() is unknown
the -Wformat-overflow checker uses get_range_strlen() to compute
the length of the longest string that fits in an array reference
by the subexpression (i.e., sizeof array - 1) and uses it to
issue warnings.  This works with member arrays but because of
a bug/limitation it doesn't work for non-member arrays.  Bug
79538 tracks this.  So the warning above suggests your change
has fixed the problem -- good work! :)

Martin