Re: [PATCH 2/6] New warnings -Wstring-plus-{char, int} (PR c++/62181)

[Xi Ruoyao <ryxi at stu dot xidian dot edu dot cn>]

On 2017-06-19 12:44 -0600, Martin Sebor wrote:
> On 06/19/2017 11:28 AM, Xi Ruoyao wrote:
> > On 2017-06-19 10:51 -0600, Martin Sebor wrote:
> > > On 06/11/2017 07:32 PM, Xi Ruoyao wrote:
> > > > This patch adds warning option -Wstring-plus-int for C/C++.
> > > > 
> > > > gcc/ChangeLog:
> > > > 
> > > > 2017-06-12  Xi Ruoyao  <ryxi@stu.xidian.edu.cn>
> > > > 
> > > > 	* c-family/c.opt: New option -Wstring-plus-int.
> > > > 	* c-family/c-common.c (pointer_int_sum): Checking for
> > > > 	-Wstring-plus-int.
> > > 
> > > This is a very useful warning but I would suggest to word it
> > > in terms of what it actually does rather than what it might be
> > > intended to do.  E.g., for
> > > 
> > >    const char *p = "123" + 7;
> > > 
> > > issue
> > > 
> > >    warning: offset 7 exceeds the upper bound 3 of the array
> > > 
> > > rather than
> > > 
> > >    warning: adding 'int' to a string does not append to the string
> > > 
> > > (I have trouble envisioning on what grounds someone might expect
> > > the addition to have this effect.)
> > 
> > How about something like `const char *p = "123" + getchar();` ?
> 
> I'm not sure I correctly understand the question (or whether
> it's meant in response to my comment in parentheses) but let
> me clarify what I meant.
> 
> In my view, the group of C++ (and certainly C) programmers who
> might expect "123" + i to append the string representation of
> the integer result to a string literal isn't significant enough
> to focus the warning on.
> 
> Whether or not the addition is valid depends on the value of
> the integer operand.  There are three sets of cases where the
> addition is or may be invalid:
> 
> 1) the integer operand is an out of bounds constant
> 2) the integer operand's non-constant value or the lower bound
>     of its range is known to be out of bounds,
> 3) the lower bound of the operand's range is in bounds but
>     the upper bound is out of bounds (as in the getchar example).
> 
> (1) can be handled with lexical analysis alone (as in you parch)
> but it's prone to a high rate of false negatives.  (3) can also
> be handled by lexical analysis alone but it's prone to a high
> rate of false positives.  (2) has no false positives but some
> false negatives.  It can only be detected with optimization.
> 
> With that in mind the warning would serve a greater purpose
> by being aimed more broadly and describing the nature of the
> error: forming an invalid pointer.  I believe it would best
> be implemented analogously to or even integrated into
> -Warray-bounds.  I.e., I suggest covering set (2) above.

I created PR 81172.  For const char *p = "123" + 'c' we should have:

    warning: offset 99 is above array bounds, the behaviour is
    undefined [-Warray-bounds]
        const char *p = "123" + 'c';

and perhaps (for the case the pointer operand is a string or a string
pointer):
    note: adding integer to a string does not append to the string.

> > 
> > I'd like this for -Wstring-plus-int=1:
> > 
> >     warning: adding 'int' to a string does not append to the string
> >     [-Wstring-plus-int=]
> >         const char *p = "123" + 7;
> >                               ^
> >     note: offset 7 exceeds the size 4 of the string, using the result
> >     may lead to undefined behaviour.
> 
> The addition itself is undefined, regardless of whether or not
> the result is used.
> 
> > 
> > (Clang permits "123" + 4 since its result is well defined in standard.
> > Maybe we could permit "123" + 3 only.)
> 
> "123" is an array of 4 elements, with "123" + 4 pointing just past
> the last (NUL) element.  It's valid to form a pointer past the last
> element of an array and warning about it would likely be viewed as
> a false positive (certainly if it were an out-of-bounds type of
> warning).
> 
> Martin
-- 
Xi Ruoyao <ryxi@stu.xidian.edu.cn>
School of Aerospace Science and Technology, Xidian University