From: Martin Sebor <msebor@gmail.com>
To: Jeff Law <law@redhat.com>,
Gcc Patch List <gcc-patches@gcc.gnu.org>,
Jakub Jelinek <jakub@redhat.com>
Subject: Re: [PATCH] correct -Wrestrict handling of arrays of arrays (PR 84095)
Date: Thu, 15 Feb 2018 17:48:00 -0000 [thread overview]
Message-ID: <b3bf7346-2f95-2057-7da4-cf4a000627d5@gmail.com> (raw)
In-Reply-To: <a73a3daa-5d52-69ca-e7c2-2d87dfa0ad5d@redhat.com>
On 02/13/2018 11:14 PM, Jeff Law wrote:
> On 02/01/2018 04:45 PM, Martin Sebor wrote:
>> The previous patch didn't resolve all the false positives
>> in the Linux kernel. The attached is an update that fixes
>> the remaining one having to do with multidimensional array
>> members:
>>
>> struct S { char a[2][4]; };
>>
>> void f (struct S *p, int i)
>> {
>> strcpy (p->a[0], "012");
>> strcpy (p->a[i] + 1, p->a[0]); // false positive here
>> }
>>
>> In the process of fixing this I also made a couple of minor
>> restructuring changes to the builtin_memref constructor to
>> in order to make the code easier to follow: I broke it out
>> into a couple of helper functions and called those.
>>
>> As with the first revision of the patch, this one is also
>> meant to be applied on top of
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html
>>
>> Sorry about the late churn. Even though I tested the original
>> implementation with the Linux kernel the bugs were only exposed
>> non-default configurations that I didn't build.
>>
>> Jakub, you had concerns about the code in the constructor
>> and about interpreting the offsets in the diagnostics.
>> I tried to address those in the patch. Please review
>> the changes and let me know if you have any further comments.
>>
>> Thanks
>> Martin
>>
>> On 01/30/2018 04:19 PM, Martin Sebor wrote:
>>> Testing GCC 8 with recent Linux kernel sources has uncovered
>>> a bug in the handling of arrays of arrays by the -Wrestrict
>>> checker where it fails to take references to different array
>>> elements into consideration, issuing false positives.
>>>
>>> The attached patch corrects this mistake.
>>>
>>> In addition, to make warnings involving excessive offset bounds
>>> more meaningful (less confusing), I've made a cosmetic change
>>> to constrain them to the bounds of the accessed object. I've
>>> done this in response to multiple comments indicating that
>>> the warnings are hard to interpret. This change is meant to
>>> be applied on top of the patch for bug 83698 (submitted mainly
>>> to improve the readability of the offsets):
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html
>>>
>>> Martin
>>
>>
>> gcc-84095.diff
>>
>>
>> PR middle-end/84095 - false-positive -Wrestrict warnings for memcpy within array
>>
>> gcc/ChangeLog:
>>
>> PR middle-end/84095
>> * gimple-ssa-warn-restrict.c (builtin_memref::extend_offset_range): New.
>> (builtin_memref::set_base_and_offset): Same. Handle inner references.
>> (builtin_memref::builtin_memref): Factor out parts into
>> set_base_and_offset and call it.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR middle-end/84095
>> * c-c++-common/Warray-bounds-3.c: Adjust text of expected warnings.
>> * c-c++-common/Wrestrict.c: Same.
>> * gcc.dg/Wrestrict-6.c: Same.
>> * gcc.dg/Warray-bounds-27.c: New test.
>> * gcc.dg/Wrestrict-8.c: New test.
>> * gcc.dg/Wrestrict-9.c: New test.
>> * gcc.dg/pr84095.c: New test.
>>
>> diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
>> index 528eb5b..367e05f 100644
>> --- a/gcc/gimple-ssa-warn-restrict.c
>> +++ b/gcc/gimple-ssa-warn-restrict.c
>
>> + else if (gimple_nop_p (stmt))
>> + expr = SSA_NAME_VAR (expr);
>> + else
>> + {
>> + base = expr;
>> + return;
>> }
> This looks odd. Can you explain what you're trying to do here?
>
> I'm not offhand why you'd ever want to extract SSA_NAME_VAR. In general
> it's primary use is for dumps and debugging info. I won't quite go so
> far as to say using it for anything else is wrong, but it's certainly
> something you ought to explain.
It appears to be dead code. Nothing in the GCC test suite hits
this code. It's most likely a vestige of an approach I tried
that didn't work and that I ended up doing differently and forgot
to remove. I'll remove it before committing.
> The rest looks fairly reasonable. It's a bit hard to follow, but I
> don't think we should do another round of refactoring at this stage.
Is the patch good to commit then with the unused code above
removed?
Martin
next prev parent reply other threads:[~2018-02-15 17:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-30 23:37 Martin Sebor
2018-02-01 23:45 ` Martin Sebor
2018-02-09 2:46 ` Martin Sebor
2018-02-14 6:14 ` Jeff Law
2018-02-15 17:48 ` Martin Sebor [this message]
2018-02-16 23:39 ` Jeff Law
2018-02-23 3:17 ` Siddhesh Poyarekar
2018-02-23 15:52 ` Martin Sebor
2018-02-23 16:19 ` Siddhesh Poyarekar
2018-02-23 16:49 ` Jakub Jelinek
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=b3bf7346-2f95-2057-7da4-cf4a000627d5@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=law@redhat.com \
/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).