From: Jeff Law <law@redhat.com>
To: Martin Sebor <msebor@gmail.com>, Richard Biener <rguenther@suse.de>
Cc: Gcc Patch List <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455)
Date: Tue, 31 Oct 2017 04:32:00 -0000 [thread overview]
Message-ID: <a770a2a7-f548-7d32-eaee-7358f2c4cea9@redhat.com> (raw)
In-Reply-To: <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com>
On 10/30/2017 05:29 PM, Martin Sebor wrote:
> On 10/30/2017 03:48 PM, Jeff Law wrote:
>> On 10/30/2017 09:19 AM, Martin Sebor 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 forwpropI think part of
>>> the problem is there isn't a well defined place to do
>> this kind of warning. I suspect it's currently in VRP simply because
>> that is where we had range information in the past. It's still the
>> location with the most accurate range information.
>>
>> In a world where we have an embedded context sensitive range analysis
>> engine, we should *really* look at pulling the out of bounds array
>> warnings out of any optimization pass an have a distinct pass to deal
>> with them.
>>
>> I guess in the immediate term the question I would ask Martin is what is
>> it about forwprop that makes it interesting? Is it because of the
>> lowering issues we touched on last week? If so I wonder if we could
>> recreate an array form from a MEM_REF for the purposes of optimization.
>> Or if we could just handle MEM_REFs better within the existing warning.
>
> I put it in forwprop only because that was the last stage where
> there's still enough context before the POINTER_PLUS_EXPR is
> folded into MEM_REF to tell an offset from the beginning of
> a subobject from the one from the beginning of the bigger object
> of which the subobject is a member. I certainly don't mind moving
> it somewhere else more appropriate if this isn't ideal, just as
> long it doesn't cripple the detection (e.g., as long as we still
> have range information).
Understood.
[ ... ]
>
> I of course don't want to break anything. I didn't see any fallout
> in my testing and I normally test all the front ends, including Ada,
> but let me check to make sure I tested it this time (I had made some
> temporary changes to my build script and may have disabled it.)Â Let
> me double check it after I get back from my trip.
No worries. Hopefully by the time you're back I'll have something
publishable on the ripping apart tree-vrp front and we can prototype the
effectiveness of doing this kind of stuff outside tree-vrp.c
We should also revisit Aldy's work from last year which started the
whole effort around fixing how we deal with out out of bounds index
testing. He had a version which ran outside tree-vrp.c but used the
same basic structure and queried range data for the index. I've got a
copy here we can poke at.
jeff
next prev parent reply other threads:[~2017-10-31 3:40 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
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 [this message]
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=a770a2a7-f548-7d32-eaee-7358f2c4cea9@redhat.com \
--to=law@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@gmail.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).