From: Martin Sebor <msebor@gmail.com>
To: Richard Biener <rguenther@suse.de>
Cc: Gcc Patch List <gcc-patches@gcc.gnu.org>, Jeff Law <law@redhat.com>
Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455)
Date: Mon, 30 Oct 2017 20:40:00 -0000 [thread overview]
Message-ID: <ba57f7d9-fd4b-0692-462f-cf4fcf9b6f93@gmail.com> (raw)
In-Reply-To: <C174CF6C-E18C-4575-AA5E-AB7D52A6C1D8@suse.de>
On 10/30/2017 01:53 PM, Richard Biener wrote:
> On October 30, 2017 4:19:25 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
>> On 10/30/2017 05:45 AM, Richard Biener wrote:
>>> On Sun, 29 Oct 2017, Martin Sebor wrote:
>>>
>>>> In my work on -Wrestrict, to issue meaningful warnings, I found
>>>> it important to detect both out of bounds array indices as well
>>>> as offsets in calls to restrict-qualified functions like strcpy.
>>>> GCC already detects some of these cases but my tests for
>>>> the enhanced warning exposed a few gaps.
>>>>
>>>> The attached patch enhances -Warray-bounds to detect more instances
>>>> out-of-bounds indices and offsets to member arrays and non-array
>>>> members. For example, it detects the out-of-bounds offset in the
>>>> call to strcpy below.
>>>>
>>>> The patch is meant to be applied on top posted here but not yet
>>>> committed:
>>>> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01304.html
>>>>
>>>> Richard, since this also touches tree-vrp.c I look for your
>> comments.
>>>
>>> You fail to tell what you are changing and why - I have to reverse
>>> engineer this from the patch which a) isn't easy in this case, b)
>> feels
>>> like a waste of time. Esp. since the patch does many things.
>>>
>>> My first question is why do you add a warning from forwprop? It
>>> _feels_ like you're trying to warn about arbitrary out-of-bound
>>> addresses at the point they are folded to MEM_REFs. And it looks
>>> like you're warning about pointer arithmetic like &p->a + 6.
>>> That doesn't look correct to me. Pointer arithmetic in GIMPLE
>>> is not restricted to operate within fields that are appearantly
>>> accessed here - the only restriction is with respect to the
>>> whole underlying pointed-to-object.
>>>
>>> By doing the warning from forwprop you'll run into all such cases
>>> introduced by GCC itself during quite late optimization passes.
>>
>> I haven't run into any such cases. What would a more appropriate
>> place to detect out-of-bounds offsets? I'm having a hard time
>> distinguishing what is appropriate and what isn't. For instance,
>> if it's okay to detect some out of bounds offsets/indices in vrp
>> why is it wrong to do a better job of it in forwprop?
>>
>>>
>>> You're trying to re-do __builtin_object_size even when that wasn't
>>> used.
>>
>> That's not the quite my intent, although it is close.
>>
>>>
>>> So it looks like you're on the wrong track. Yes,
>>>
>>> strcpy (p->a + 6, "y");
>>>
>>> _may_ be "invalid" C (I'm not even sure about that!) but it
>>> is certainly not invalid GIMPLE.
>>
>> Adding (or subtracting) an integer to/from a pointer to an array
>> is defined in both C and C++ only if the result points to an element
>> of the array or just past the last element of the array. Otherwise
>> it's undefined. (A non-array object is considered an array of one
>> for this purpose.)
>
> On GIMPLE this is indistinguishable from (short *) (p->a) + 3.
Sure, they're both the same:
pa_3 = &p_2(D)->a;
_1 = pa_3 + 6;
and that's fine because the implementation of the warning sees and
uses the byte offset from the beginning of a, so I don't understand
the problem you are pointing out. Can you clarify what you mean?
Martin
>
> GIMPLE is neither C nor C++.
>
> Richard.
>
>>
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Jeff, this is the enhancement you were interested in when we spoke
>>>> last week.
>>>>
>>>> Thanks
>>>> Martin
>>>>
>>>> $ cat a.c && gcc -O2 -S -Wall a.c
>>>> struct A { char a[4]; void (*pf)(void); };
>>>>
>>>> void f (struct A *p)
>>>> {
>>>> p->a[5] = 'x'; // existing -Warray-bounds
>>>>
>>>> strcpy (p->a + 6, "y"); // enhanced -Warray-bounds
>>>> }
>>>>
>>>> a.c: In function âfâ:
>>>> a.c:7:3: warning: offset 6 is out of bounds of âchar[4]â
>> [-Warray-bounds]
>>>> strcpy (p->a + 6, "y");
>>>> ^~~~~~~~~~~~~~~~~~~~~~
>>>> a.c:1:17: note: member declared here
>>>> struct A { char a[4]; void (*pf)(void); };
>>>> ^
>>>> a.c:5:7: warning: array subscript 5 is above array bounds of
>> âchar[4]â
>>>> [-Warray-bounds]
>>>> p->a[5] = 'x';
>>>> ~~~~^~~
>
next prev parent reply other threads:[~2017-10-30 20:13 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-29 16:15 Martin Sebor
2017-10-30 11:55 ` Richard Biener
2017-10-30 15:21 ` Martin Sebor
2017-10-30 19:59 ` Richard Biener
2017-10-30 20:40 ` Martin Sebor [this message]
2017-10-30 21:23 ` Richard Biener
2017-10-30 21:49 ` Martin Sebor
2017-11-02 11:48 ` Richard Biener
2017-11-10 1:12 ` Jeff Law
2017-11-10 8:25 ` Richard Biener
2017-11-14 0:04 ` Jeff Law
2017-11-14 9:11 ` Richard Biener
2017-11-15 1:52 ` Jeff Law
2017-11-14 5:22 ` Martin Sebor
2017-11-14 9:13 ` Richard Biener
2017-11-15 1:54 ` Jeff Law
2017-10-30 22:16 ` Jeff Law
2017-10-30 23:30 ` Martin Sebor
2017-10-31 4:32 ` Jeff Law
2017-11-01 22:21 ` Martin Sebor
2017-11-02 11:27 ` Richard Biener
2017-10-30 22:16 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ba57f7d9-fd4b-0692-462f-cf4fcf9b6f93@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).