Message ID | Yh5o8gZ3FoavGg20@tucnak |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 756FA3858C2C for <patchwork@sourceware.org>; Tue, 1 Mar 2022 18:42:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 756FA3858C2C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1646160152; bh=dhfxtNNhsvDijiEJ/E8O9LG2gXLj8kLu7fL2LzC9Odo=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=AgrD7w9lfkQy58oC4UETVNbkK21eLEtYoZAW+/5sw3gbUJrkb1h01EPkblPj3xDsT 0cAZknCIX6RjdRSWbAeoiULUboxtXADZWMtlVILH4gpS5xZflirU4wRaZKrHsJMK8b XJLhxU6jTo3u5NPLJulkEOwF32OlcRubwPCz4QL8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4DCF63858D20 for <gcc-patches@gcc.gnu.org>; Tue, 1 Mar 2022 18:42:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4DCF63858D20 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-659--uqX4VInOyK3nxN70_cCwQ-1; Tue, 01 Mar 2022 13:41:59 -0500 X-MC-Unique: -uqX4VInOyK3nxN70_cCwQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 83E861006AA5; Tue, 1 Mar 2022 18:41:58 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.57]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0142A804F1; Tue, 1 Mar 2022 18:41:57 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 221Ift5P3311406 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 1 Mar 2022 19:41:55 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 221IfsIl3311405; Tue, 1 Mar 2022 19:41:54 +0100 Date: Tue, 1 Mar 2022 19:41:54 +0100 To: Richard Biener <rguenther@suse.de>, Jeff Law <jeffreyalaw@gmail.com> Subject: [PATCH] warn-access: Fix up check_pointer_uses [PR104715] Message-ID: <Yh5o8gZ3FoavGg20@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org, Martin Sebor <msebor@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
warn-access: Fix up check_pointer_uses [PR104715]
|
|
Commit Message
Jakub Jelinek
March 1, 2022, 6:41 p.m. UTC
Hi! The following testcase emits bogus -Wdangling-pointer warnings. The bug is that when it sees that ptr immediate use is a call that returns one of its arguments, it will assume that the return value is based on ptr, but that is the case only if ptr is passed to the argument that is actually returned (so e.g. for memcpy the first argument, etc.). When the builtins guarantee e.g. that the result is based on the first argument (either ERF_RETURNS_ARG 0 in which case it will always just returns the first argument as is, or when it is something like strstr or strpbrk or mempcpy that it returns some pointer based on the first argument), it means the result is not based on second or following argument if any. The second hunk fixes this. The first hunk just removes an unnecessary TREE_CODE check, the code only pushes SSA_NAMEs into the pointers vector and if it didn't, it uses FOR_EACH_IMM_USE_FAST (use_p, iter, ptr) a few lines below this, which of course requires that ptr is a SSA_NAME. Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't a SSA_NAME, we'd ICE. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2022-03-01 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/104715 * gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't unnecessarily test if ptr is a SSA_NAME, it has to be. Only push lhs of a call if gimple_call_return_arg is equal to ptr, not just when it is non-NULL. * c-c++-common/Wdangling-pointer-7.c: New test. Jakub
Comments
On 3/1/22 11:41, Jakub Jelinek wrote: > Hi! > > The following testcase emits bogus -Wdangling-pointer warnings. > The bug is that when it sees that ptr immediate use is a call that > returns one of its arguments, it will assume that the return value > is based on ptr, but that is the case only if ptr is passed to the > argument that is actually returned (so e.g. for memcpy the first argument, > etc.). When the builtins guarantee e.g. that the result is based on the > first argument (either ERF_RETURNS_ARG 0 in which case it will always > just returns the first argument as is, or when it is something like > strstr or strpbrk or mempcpy that it returns some pointer based on the > first argument), it means the result is not based on second or following > argument if any. The second hunk fixes this. > > The first hunk just removes an unnecessary TREE_CODE check, the code only > pushes SSA_NAMEs into the pointers vector and if it didn't, it uses > FOR_EACH_IMM_USE_FAST (use_p, iter, ptr) > a few lines below this, which of course requires that ptr is a SSA_NAME. > Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't > a SSA_NAME, we'd ICE. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Thanks for the fix. It makes sense to me. Besides the test for the false positives I would suggest to add one to verify that using the first argument to a strstr() call is diagnosed if it's dangling (both as is, as well as with an offset from the first element). There are tests for memchr and strchr in the -Wdangling-pointer test suite but none for strstr. Martin > > 2022-03-01 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/104715 > * gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't > unnecessarily test if ptr is a SSA_NAME, it has to be. Only push lhs > of a call if gimple_call_return_arg is equal to ptr, not just when it > is non-NULL. > > * c-c++-common/Wdangling-pointer-7.c: New test. > > --- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 > +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 > @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple > for (unsigned i = 0; i != pointers.length (); ++i) > { > tree ptr = pointers[i]; > - if (TREE_CODE (ptr) == SSA_NAME > - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) > + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) > /* Avoid revisiting the same pointer. */ > continue; > > @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple > > if (gcall *call = dyn_cast <gcall *>(use_stmt)) > { > - if (gimple_call_return_arg (call)) > + if (gimple_call_return_arg (call) == ptr) > if (tree lhs = gimple_call_lhs (call)) > if (TREE_CODE (lhs) == SSA_NAME) > pointers.safe_push (lhs); > --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 > +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 > @@ -0,0 +1,36 @@ > +/* PR tree-optimization/104715 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wdangling-pointer" } */ > + > +char * > +foo (char *p) > +{ > + { > + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; > + char *r = q; > + p = __builtin_strcat (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > + > +char * > +bar (char *p) > +{ > + { > + char q[] = "0123456789"; > + char *r = q; > + p = __builtin_strstr (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > + > +char * > +baz (char *p) > +{ > + { > + char q[] = "0123456789"; > + char *r = q; > + p = __builtin_strpbrk (p, r); > + } > + return p; /* { dg-bogus "using dangling pointer" } */ > +} > > Jakub >
> Am 01.03.2022 um 20:08 schrieb Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org>: > > On 3/1/22 11:41, Jakub Jelinek wrote: >> Hi! >> The following testcase emits bogus -Wdangling-pointer warnings. >> The bug is that when it sees that ptr immediate use is a call that >> returns one of its arguments, it will assume that the return value >> is based on ptr, but that is the case only if ptr is passed to the >> argument that is actually returned (so e.g. for memcpy the first argument, >> etc.). When the builtins guarantee e.g. that the result is based on the >> first argument (either ERF_RETURNS_ARG 0 in which case it will always >> just returns the first argument as is, or when it is something like >> strstr or strpbrk or mempcpy that it returns some pointer based on the >> first argument), it means the result is not based on second or following >> argument if any. The second hunk fixes this. >> The first hunk just removes an unnecessary TREE_CODE check, the code only >> pushes SSA_NAMEs into the pointers vector and if it didn't, it uses >> FOR_EACH_IMM_USE_FAST (use_p, iter, ptr) >> a few lines below this, which of course requires that ptr is a SSA_NAME. >> Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't >> a SSA_NAME, we'd ICE. >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Ok Richard > Thanks for the fix. It makes sense to me. Besides the test for > the false positives I would suggest to add one to verify that using > the first argument to a strstr() call is diagnosed if it's dangling > (both as is, as well as with an offset from the first element). > There are tests for memchr and strchr in the -Wdangling-pointer > test suite but none for strstr. > > Martin > >> 2022-03-01 Jakub Jelinek <jakub@redhat.com> >> PR tree-optimization/104715 >> * gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't >> unnecessarily test if ptr is a SSA_NAME, it has to be. Only push lhs >> of a call if gimple_call_return_arg is equal to ptr, not just when it >> is non-NULL. >> * c-c++-common/Wdangling-pointer-7.c: New test. >> --- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 >> +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 >> @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple >> for (unsigned i = 0; i != pointers.length (); ++i) >> { >> tree ptr = pointers[i]; >> - if (TREE_CODE (ptr) == SSA_NAME >> - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) >> + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) >> /* Avoid revisiting the same pointer. */ >> continue; >> @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple >> if (gcall *call = dyn_cast <gcall *>(use_stmt)) >> { >> - if (gimple_call_return_arg (call)) >> + if (gimple_call_return_arg (call) == ptr) >> if (tree lhs = gimple_call_lhs (call)) >> if (TREE_CODE (lhs) == SSA_NAME) >> pointers.safe_push (lhs); >> --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 >> +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 >> @@ -0,0 +1,36 @@ >> +/* PR tree-optimization/104715 */ >> +/* { dg-do compile } */ >> +/* { dg-options "-Wdangling-pointer" } */ >> + >> +char * >> +foo (char *p) >> +{ >> + { >> + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; >> + char *r = q; >> + p = __builtin_strcat (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> + >> +char * >> +bar (char *p) >> +{ >> + { >> + char q[] = "0123456789"; >> + char *r = q; >> + p = __builtin_strstr (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> + >> +char * >> +baz (char *p) >> +{ >> + { >> + char q[] = "0123456789"; >> + char *r = q; >> + p = __builtin_strpbrk (p, r); >> + } >> + return p; /* { dg-bogus "using dangling pointer" } */ >> +} >> Jakub >
On Tue, Mar 01, 2022 at 12:07:49PM -0700, Martin Sebor wrote: > Thanks for the fix. It makes sense to me. Besides the test for > the false positives I would suggest to add one to verify that using > the first argument to a strstr() call is diagnosed if it's dangling > (both as is, as well as with an offset from the first element). > There are tests for memchr and strchr in the -Wdangling-pointer > test suite but none for strstr. Thanks for adding that test. Note, as I wrote in the PR, I think we should handle BUILT_IN_STRPBRK like BUILT_IN_STRSTR in pass_waccess::gimple_call_return_arg, but as that would emit further warnings, I think that has to be a GCC 13 material. Jakub
--- gcc/gimple-ssa-warn-access.cc.jj 2022-02-28 16:22:40.860520930 +0100 +++ gcc/gimple-ssa-warn-access.cc 2022-02-28 16:55:01.242272499 +0100 @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple for (unsigned i = 0; i != pointers.length (); ++i) { tree ptr = pointers[i]; - if (TREE_CODE (ptr) == SSA_NAME - && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) + if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr))) /* Avoid revisiting the same pointer. */ continue; @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple if (gcall *call = dyn_cast <gcall *>(use_stmt)) { - if (gimple_call_return_arg (call)) + if (gimple_call_return_arg (call) == ptr) if (tree lhs = gimple_call_lhs (call)) if (TREE_CODE (lhs) == SSA_NAME) pointers.safe_push (lhs); --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj 2022-02-28 17:09:09.906355082 +0100 +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c 2022-02-28 17:03:50.533839892 +0100 @@ -0,0 +1,36 @@ +/* PR tree-optimization/104715 */ +/* { dg-do compile } */ +/* { dg-options "-Wdangling-pointer" } */ + +char * +foo (char *p) +{ + { + char q[61] = "012345678901234567890123456789012345678901234567890123456789"; + char *r = q; + p = __builtin_strcat (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +} + +char * +bar (char *p) +{ + { + char q[] = "0123456789"; + char *r = q; + p = __builtin_strstr (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +} + +char * +baz (char *p) +{ + { + char q[] = "0123456789"; + char *r = q; + p = __builtin_strpbrk (p, r); + } + return p; /* { dg-bogus "using dangling pointer" } */ +}