From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59004 invoked by alias); 24 Aug 2018 05:58:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 58988 invoked by uid 89); 24 Aug 2018 05:58:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Aug 2018 05:58:11 +0000 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5D30083F3C; Fri, 24 Aug 2018 05:58:10 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-19.rdu2.redhat.com [10.10.112.19]) by smtp.corp.redhat.com (Postfix) with ESMTP id EBE192010CE7; Fri, 24 Aug 2018 05:58:08 +0000 (UTC) Subject: Re: [PATCH] Improve checks in c_strlen (PR 87053) To: Bernd Edlinger , Martin Sebor , "gcc-patches@gcc.gnu.org" , Richard Biener References: <06f46611-3f6a-e0e9-9a2c-b505dc5ccfff@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <4fc71412-4c7e-7024-79b5-da999cb9977e@redhat.com> Date: Fri, 24 Aug 2018 05:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg01513.txt.bz2 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. >