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

[Martin Sebor <msebor at gmail dot com>]

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