From: Jeff Law <law@redhat.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>,
Martin Sebor <msebor@gmail.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
Richard Biener <rguenther@suse.de>
Subject: Re: [PATCH] Improve checks in c_strlen (PR 87053)
Date: Fri, 24 Aug 2018 05:58:00 -0000 [thread overview]
Message-ID: <4fc71412-4c7e-7024-79b5-da999cb9977e@redhat.com> (raw)
In-Reply-To: <AM5PR0701MB2657A8122E7EE7149EA2460CE4370@AM5PR0701MB2657.eurprd07.prod.outlook.com>
On 08/23/2018 03:27 AM, Bernd Edlinger wrote:
> On 08/22/18 18:28, Martin Sebor wrote:
>> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>>> Hi!
>>>
>>>
>>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>>> wrong code bug.
>>>
>>> Unfortunately this patch alone is not sufficient to fix the problem,
>>> but also the patch for PR 86714 that hardens c_getstr is necessary
>>> to prevent the wrong folding.
>>>
>>>
>>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>>> Is it OK for trunk?
>>
>> This case is also the subject of the patch I submitted back in
>> July for 86711/86714 and 86552. With it, GCC avoid folding
>> the strlen call early and warns for the missing nul:
>>
>> warning: â__builtin_strlenâ argument missing terminating nul [-Wstringop-overflow=]
>> Â Â if (__builtin_strlen (u.z) != 7)
>> Â Â Â Â Â Â ^~~~~~~~~~~~~~~~
>>
>> The patch doesn't doesn't prevent all such strings from being
>> folded and it eventually lets fold_builtin_strlen() do its thing:
>>
>> Â Â Â Â Â /* To avoid warning multiple times about unterminated
>> Â Â Â Â Â Â Â Â arrays only warn if its length has been determined
>>         and is being folded to a constant. */
>> Â Â Â Â Â if (nonstr)
>> Â Â Â Â Â Â Â warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
>>
>> Â Â Â Â Â return fold_convert_loc (loc, type, len);
>>
>> Handling this case is a matter of avoiding the folding here as
>> well and moving the warning later.
>>
>> Since my patch is still in the review queue and does much more
>> than just prevent folding of non-nul terminated arrays it should
>> be reviewed first.
>>
>
> Hmmm, now you made me curious.
>
> So I tried to install your patch (I did this on r263508
> since it does not apply to trunk, one thing I noted is
> that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c
> I did not check if they are identical or not).
>
> So I tried the test case from this PR on the compiler built with your patch:
>
> $ cat cat pr87053.c
> /* PR middle-end/87053 */
>
> const union
> { struct {
> char x[4];
> char y[4];
> };
> struct {
> char z[8];
> };
> } u = {{"1234", "567"}};
>
> int main ()
> {
> if (__builtin_strlen (u.z) != 7)
> __builtin_abort ();
> }
> $ gcc -S pr87053.c
> pr87053.c: In function 'main':
> pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=]
> 15 | if (__builtin_strlen (u.z) != 7)
> | ^~~~~~~~~~~~~~~~
> pr87053.c:11:3: note: referenced argument declared here
> 11 | } u = {{"1234", "567"}};
> | ^
> $ cat pr87053.s
> .file "pr87053.c"
> .text
> .globl u
> .section .rodata
> .align 8
> .type u, @object
> .size u, 8
> u:
> .ascii "1234"
> .string "567"
> .text
> .globl main
> .type main, @function
> main:
> .LFB0:
> .cfi_startproc
> pushq %rbp
> .cfi_def_cfa_offset 16
> .cfi_offset 6, -16
> movq %rsp, %rbp
> .cfi_def_cfa_register 6
> call abort
> .cfi_endproc
> .LFE0:
> .size main, .-main
> .ident "GCC: (GNU) 9.0.0 20180813 (experimental)"
> .section .note.GNU-stack,"",@progbits
>
>
> So we get a warning, and still wrong code.
>
> That is the reason why I think this patch of yours adds
> confusion by trying to fix everything in one step.
>
> And I would like you to think of ways how to solve
> a problem step by step.
>
> And at this time, sorry, we should restore correctness issues.
> And fix wrong-code issues.
> If possible without breaking existing warnings, yes.
> But no new warnings, sorry again.
Just a note, Martin's most fix for 86711/86714 fixes codegen issues
without breaking existing warnings or adding new warnings. The new
warnings were broken out into follow-up patches.
jeff
> Bernd.
>
next prev parent reply other threads:[~2018-08-24 5:58 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-22 14:42 Bernd Edlinger
2018-08-22 16:28 ` Martin Sebor
2018-08-23 9:27 ` Bernd Edlinger
2018-08-24 5:58 ` Jeff Law [this message]
2018-08-24 12:31 ` Bernd Edlinger
2018-08-24 5:56 ` Jeff Law
2018-08-29 21:38 ` 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=4fc71412-4c7e-7024-79b5-da999cb9977e@redhat.com \
--to=law@redhat.com \
--cc=bernd.edlinger@hotmail.de \
--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).