Message ID | 20220425095435.226C013AE1@imap2.suse-dmz.suse.de |
---|---|
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 94F78385843E for <patchwork@sourceware.org>; Mon, 25 Apr 2022 09:55:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 94F78385843E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1650880505; bh=36YVENPF1oEZmn5kGbQF4a7tdmK6Kg6W7e59whZTRz8=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=GbtTh8hpbHlh/ZVfXRsvIGZWW5pgPBb+5Hv7Y+HZWYGjgcBvoRAaMTh1eBKUFZMo4 2btlIiqudiUa/ZBPPkn/Ixs7kHrtxeMhLpQHbLWvAdW162OpaRsW0j5A68RmqQOd5h 9/mYEbo2J4QYKEoVqFPL+j+IhD8lkAIitWbBhJ4Q= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 83D123858D28 for <gcc-patches@gcc.gnu.org>; Mon, 25 Apr 2022 09:54:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 83D123858D28 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 3B629210E7; Mon, 25 Apr 2022 09:54:35 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 226C013AE1; Mon, 25 Apr 2022 09:54:35 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id AH9SB9tvZmL/JQAAMHmgww (envelope-from <rguenther@suse.de>); Mon, 25 Apr 2022 09:54:35 +0000 Date: Mon, 25 Apr 2022 11:54:34 +0200 (CEST) To: gcc-patches@gcc.gnu.org Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling pointer diags MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Message-Id: <20220425095435.226C013AE1@imap2.suse-dmz.suse.de> X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP 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: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Richard Biener <rguenther@suse.de> Cc: Jakub Jelinek <jakub@redhat.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 |
middle-end/104492 - avoid all equality compare dangling pointer diags
|
|
Commit Message
Richard Biener
April 25, 2022, 9:54 a.m. UTC
The following extends the equality compare dangling pointer diagnostics suppression for uses following free or realloc to also cover those following invalidation of auto variables via CLOBBERs. That avoids diagnosing idioms like return std::find(std::begin(candidates), std::end(candidates), s) != std::end(candidates); for auto candidates which are prone to forwarding of the final comparison across the storage invalidation as then seen by the late run access warning pass. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? Thanks, Richard. 2022-04-25 Richard Biener <rguenther@suse.de> PR middle-end/104492 * gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): Exclude equality compare diagnostics for all kind of invalidations. * c-c++-common/Wdangling-pointer.c: Adjust for changed suppression. * c-c++-common/Wdangling-pointer-2.c: Likewise. --- gcc/gimple-ssa-warn-access.cc | 14 +++++--------- gcc/testsuite/c-c++-common/Wdangling-pointer-2.c | 4 ++-- gcc/testsuite/c-c++-common/Wdangling-pointer.c | 4 ++-- 3 files changed, 9 insertions(+), 13 deletions(-)
Comments
On Mon, Apr 25, 2022 at 11:54:34AM +0200, Richard Biener wrote: > The following extends the equality compare dangling pointer diagnostics > suppression for uses following free or realloc to also cover those > following invalidation of auto variables via CLOBBERs. That avoids > diagnosing idioms like > > return std::find(std::begin(candidates), std::end(candidates), s) > != std::end(candidates); > > for auto candidates which are prone to forwarding of the final > comparison across the storage invalidation as then seen by the > late run access warning pass. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > OK for trunk? > > Thanks, > Richard. > > 2022-04-25 Richard Biener <rguenther@suse.de> > > PR middle-end/104492 > * gimple-ssa-warn-access.cc > (pass_waccess::warn_invalid_pointer): Exclude equality compare > diagnostics for all kind of invalidations. > > * c-c++-common/Wdangling-pointer.c: Adjust for changed > suppression. > * c-c++-common/Wdangling-pointer-2.c: Likewise. I spoke with Martin on IRC and his comment was that this is ok but should be accompanied with a doc/invoke.texi change that clarifies that behavior in the documentation. I think that is a reasonable request. Jakub
On Wed, 27 Apr 2022, Jakub Jelinek wrote: > On Mon, Apr 25, 2022 at 11:54:34AM +0200, Richard Biener wrote: > > The following extends the equality compare dangling pointer diagnostics > > suppression for uses following free or realloc to also cover those > > following invalidation of auto variables via CLOBBERs. That avoids > > diagnosing idioms like > > > > return std::find(std::begin(candidates), std::end(candidates), s) > > != std::end(candidates); > > > > for auto candidates which are prone to forwarding of the final > > comparison across the storage invalidation as then seen by the > > late run access warning pass. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > > > OK for trunk? > > > > Thanks, > > Richard. > > > > 2022-04-25 Richard Biener <rguenther@suse.de> > > > > PR middle-end/104492 > > * gimple-ssa-warn-access.cc > > (pass_waccess::warn_invalid_pointer): Exclude equality compare > > diagnostics for all kind of invalidations. > > > > * c-c++-common/Wdangling-pointer.c: Adjust for changed > > suppression. > > * c-c++-common/Wdangling-pointer-2.c: Likewise. > > I spoke with Martin on IRC and his comment was that this is ok > but should be accompanied with a doc/invoke.texi change that clarifies > that behavior in the documentation. > I think that is a reasonable request. Like so? diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 07b440190c3..4decbf84a43 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to objects with automatic storage duration after their lifetime has ended. This includes local variables declared in nested blocks, compound literals and other unnamed temporary objects. In addition, warn about storing the address of such -objects in escaped pointers. The warning is enabled at all optimization -levels but may yield different results with optimization than without. +objects in escaped pointers. As exception we do not warn when pointers +are used in equality compares after the lifetime of the storage they point +to ended. The warning is enabled at all optimization levels but may yield +different results with optimization than without. @table @gcctabopt @item -Wdangling-pointer=1
On Wed, Apr 27, 2022 at 09:00:51AM +0200, Richard Biener wrote: > > > 2022-04-25 Richard Biener <rguenther@suse.de> > > > > > > PR middle-end/104492 > > > * gimple-ssa-warn-access.cc > > > (pass_waccess::warn_invalid_pointer): Exclude equality compare > > > diagnostics for all kind of invalidations. > > > > > > * c-c++-common/Wdangling-pointer.c: Adjust for changed > > > suppression. > > > * c-c++-common/Wdangling-pointer-2.c: Likewise. > > > > I spoke with Martin on IRC and his comment was that this is ok > > but should be accompanied with a doc/invoke.texi change that clarifies > > that behavior in the documentation. > > I think that is a reasonable request. > > Like so? > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 07b440190c3..4decbf84a43 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to > objects with automatic > storage duration after their lifetime has ended. This includes local > variables declared in nested blocks, compound literals and other unnamed > temporary objects. In addition, warn about storing the address of such > -objects in escaped pointers. The warning is enabled at all optimization > -levels but may yield different results with optimization than without. > +objects in escaped pointers. As exception we do not warn when pointers > +are used in equality compares after the lifetime of the storage they > point > +to ended. The warning is enabled at all optimization levels but may > yield > +different results with optimization than without. > > @table @gcctabopt > @item -Wdangling-pointer=1 Reading the patch again, I don't think the above is what the patch does, but furthermore, I'm not sure the patch is right. Before your change, the code was about 2 warnings, -Wuse-after-free= with levels 0 (well, -Wno-use-after-free), 1, 2, 3 where 3 is enabling equality and the warning is done when the invalidating statement is a call. Then there is code for the -Wdangling-pointer= warning with levels 0 (well, -Wno-dangling-pointer), 1, 2. Your change moves the -Wuse-after-free= stanza for both warnings including -Wuse-after-free= suppressions for both warnings and kills the -Wdangling-pointer= stanza. I think that means there would be no difference between -Wdangling-pointer=1 and -Wdangling-pointer=2 anymore and whether the warning is given for the maybe case (or equality case) would be instead determined by -Wuse-after-free= level. I'd say the right thing would be to keep the -Wuse-after-free= stuff as is and just adjust - if ((maybe && warn_dangling_pointer < 2) + if ((equality && warn_dangling_pointer < 3) + || (maybe && warn_dangling_pointer < 2) || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) return; i.e. basically introduce -Wdangling-pointer=3 level. That would need Wdangling-pointer= -C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 2) +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 3) change, documentation what the -Wdangling-pointer=3 level means in invoke.texi (similar to -Wuse-after-free=3 documentation?) and another testcase with -Wdangling-pointer=3 in dg-options where it warns also about equality. Jakub
On Wed, Apr 27, 2022 at 09:29:21AM +0200, Jakub Jelinek via Gcc-patches wrote: > I'd say the right thing would be to keep the -Wuse-after-free= stuff as is > and just adjust > - if ((maybe && warn_dangling_pointer < 2) > + if ((equality && warn_dangling_pointer < 3) > + || (maybe && warn_dangling_pointer < 2) > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > return; > i.e. basically introduce -Wdangling-pointer=3 level. That would need Or, if the intent is what you've documented that equality comparisons are never warned about, then just if (equality || (maybe && ... But I think the extra warning level might be better especially if we document that the level can have many false positives and explain why. But if somebody is willing to go through the false positives to find some needle in the haystack, let it be his choice. Jakub
On Wed, 27 Apr 2022, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 09:00:51AM +0200, Richard Biener wrote: > > > > 2022-04-25 Richard Biener <rguenther@suse.de> > > > > > > > > PR middle-end/104492 > > > > * gimple-ssa-warn-access.cc > > > > (pass_waccess::warn_invalid_pointer): Exclude equality compare > > > > diagnostics for all kind of invalidations. > > > > > > > > * c-c++-common/Wdangling-pointer.c: Adjust for changed > > > > suppression. > > > > * c-c++-common/Wdangling-pointer-2.c: Likewise. > > > > > > I spoke with Martin on IRC and his comment was that this is ok > > > but should be accompanied with a doc/invoke.texi change that clarifies > > > that behavior in the documentation. > > > I think that is a reasonable request. > > > > Like so? > > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > index 07b440190c3..4decbf84a43 100644 > > --- a/gcc/doc/invoke.texi > > +++ b/gcc/doc/invoke.texi > > @@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to > > objects with automatic > > storage duration after their lifetime has ended. This includes local > > variables declared in nested blocks, compound literals and other unnamed > > temporary objects. In addition, warn about storing the address of such > > -objects in escaped pointers. The warning is enabled at all optimization > > -levels but may yield different results with optimization than without. > > +objects in escaped pointers. As exception we do not warn when pointers > > +are used in equality compares after the lifetime of the storage they > > point > > +to ended. The warning is enabled at all optimization levels but may > > yield > > +different results with optimization than without. > > > > @table @gcctabopt > > @item -Wdangling-pointer=1 > > Reading the patch again, I don't think the above is what the patch does, > but furthermore, I'm not sure the patch is right. > > Before your change, the code was about 2 warnings, -Wuse-after-free= > with levels 0 (well, -Wno-use-after-free), 1, 2, 3 where 3 is enabling > equality and the warning is done when the invalidating statement is > a call. > > Then there is code for the -Wdangling-pointer= warning with levels 0 (well, > -Wno-dangling-pointer), 1, 2. > > Your change moves the -Wuse-after-free= stanza for both warnings including > -Wuse-after-free= suppressions for both warnings and kills the > -Wdangling-pointer= stanza. Whoops, that wasn't my itention. > I think that means there would be no > difference between -Wdangling-pointer=1 and -Wdangling-pointer=2 anymore > and whether the warning is given for the maybe case (or equality case) > would be instead determined by -Wuse-after-free= level. > > I'd say the right thing would be to keep the -Wuse-after-free= stuff as is > and just adjust > - if ((maybe && warn_dangling_pointer < 2) > + if ((equality && warn_dangling_pointer < 3) > + || (maybe && warn_dangling_pointer < 2) > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > return; > i.e. basically introduce -Wdangling-pointer=3 level. That would need > Wdangling-pointer= > -C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 2) > +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 3) > change, documentation what the -Wdangling-pointer=3 level means in > invoke.texi (similar to -Wuse-after-free=3 documentation?) and another > testcase with -Wdangling-pointer=3 in dg-options where it warns also > about equality. I didn't want to introduce -Wdangling-pointer=3 since I think the case in the PR is just the tip of the iceberg so that we suppress for equality compares would have been an implementation detail ... So I'd do the below then. I think if we want to introduce -Wdangling-pointer=3 then we should restrict =2 way more by excluding all non-memory use stmts. So we'd still cover a.x = ptr; // escape foo (ptr); // escape *ptr = ..; // store .. = *ptr; // load but not any operations on the pointer value (compare, pointer-plus, difference, masking, etc.). A simple-minded implementation would then be if ((!gimple_vuse (use_stmt) && warn_dangling_pointer < 3) || (maybe && ... I'm going to see whether there's any testsuite coverage for !gimple_vuse use_stmt. Richard. diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index 879dbcc1e52..80b5119da7d 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_ stmt, return; } - if ((maybe && warn_dangling_pointer < 2) + if (equality + || (maybe && warn_dangling_pointer < 2) || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) return; > Jakub > >
On Wed, Apr 27, 2022 at 09:43:59AM +0200, Richard Biener wrote: > but not any operations on the pointer value (compare, pointer-plus, > difference, masking, etc.). A simple-minded implementation would > then be > > if ((!gimple_vuse (use_stmt) && warn_dangling_pointer < 3) > || (maybe && ... That would consider as memory uses both cases where we actually dereference the pointer and where we store the pointer in some memory. But perhaps that would be the goal. > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index 879dbcc1e52..80b5119da7d 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple > *use_ > stmt, > return; > } > > - if ((maybe && warn_dangling_pointer < 2) > + if (equality > + || (maybe && warn_dangling_pointer < 2) > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > return; > This is fine too with the invoke.texi change and the testcases. Jakub
On Wed, 27 Apr 2022, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 09:43:59AM +0200, Richard Biener wrote: > > but not any operations on the pointer value (compare, pointer-plus, > > difference, masking, etc.). A simple-minded implementation would > > then be > > > > if ((!gimple_vuse (use_stmt) && warn_dangling_pointer < 3) > > || (maybe && ... > > That would consider as memory uses both cases where we actually dereference > the pointer and where we store the pointer in some memory. > But perhaps that would be the goal. I think that was the intention of the diagnostic, yes. I don't think it's very useful to diagnose that you add 1 to a dangling pointer, instead I hope the code will diagnose the dereference of the offsetted dangling pointer instead (I think it does, though it might miss _1 = &MEM[_2 + 4]; from a quick look, likewise _2 & ~3 (aligning a pointer) and any tricks via uintptr casting). There's some testsuite fallout with using !gimple_vuse () because when a diagnostic is suppressed we are _not_ tracking derived pointers. For c-c++-common/Wdangling-pointer.c at -O0 we diagnose void* warn_return_local_addr (void) { int *p = 0; { int a[] = { 1, 2, 3 }; p = a; } /* Unlike the above case, here the pointer is dangling when it's used. */ return p; // { dg-warning "using dangling pointer 'p' to 'a'" "array" } } _8 = p_6; // <-- here <bb3>: return _8; but if we suppress that we do not add _8 to the pointers to check. Even if we do add it we get a maybe diagnostic because the post-dominator query uses reversed arguments (oops, we should probably fix that independently). Fixing that results in 'using a dangling pointer to 'a'' (instead of naming 'p') since _8 is anonymous. When optimizing we fail to diagnose this case - we skip return stmts because of -Wreturn-local-addr but when we do not warn about the artificial SSA copy the diagnostic is lost. So that's the only "intentional" diagnostic that's skipped by !gimple_vuse (). Still it requires too much changes to code I'm not familiar with at this point. > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > > index 879dbcc1e52..80b5119da7d 100644 > > --- a/gcc/gimple-ssa-warn-access.cc > > +++ b/gcc/gimple-ssa-warn-access.cc > > @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple > > *use_ > > stmt, > > return; > > } > > > > - if ((maybe && warn_dangling_pointer < 2) > > + if (equality > > + || (maybe && warn_dangling_pointer < 2) > > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > > return; > > > > This is fine too with the invoke.texi change and the testcases. So I'm leaning towards this to fix the P1 bug and see what other cases people come up with in the real world. As said if not introducing =3 I'd not document this implementation detail (as said above we miss some cases because we fail to follow all pointer adjustments as well). I'm re-testing the variant below, not documenting the implementation detail but fixing the post-dominator queries. Richard. From 098ab3298b273a56bafe978facdc512789a7628a Mon Sep 17 00:00:00 2001 From: Richard Biener <rguenther@suse.de> Date: Mon, 25 Apr 2022 10:46:16 +0200 Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling pointer diags To: gcc-patches@gcc.gnu.org The following extends the equality compare dangling pointer diagnostics suppression for uses following free or realloc to also cover those following invalidation of auto variables via CLOBBERs. That avoids diagnosing idioms like return std::find(std::begin(candidates), std::end(candidates), s) != std::end(candidates); for auto candidates which are prone to forwarding of the final comparison across the storage invalidation as then seen by the late run access warning pass. 2022-04-25 Richard Biener <rguenther@suse.de> PR middle-end/104492 * gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): Exclude equality compare diagnostics for all kind of invalidations. (pass_waccess::check_dangling_uses): Fix post-dominator query. (pass_waccess::check_pointer_uses): Likewise. --- gcc/gimple-ssa-warn-access.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index 879dbcc1e52..39aa8186de6 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt, return; } - if ((maybe && warn_dangling_pointer < 2) + if (equality + || (maybe && warn_dangling_pointer < 2) || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) return; @@ -4241,7 +4242,7 @@ pass_waccess::check_pointer_uses (gimple *stmt, tree ptr, basic_block use_bb = gimple_bb (use_stmt); bool this_maybe = (maybe - || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, stmt_bb)); + || !dominated_by_p (CDI_POST_DOMINATORS, stmt_bb, use_bb)); warn_invalid_pointer (*use_p->use, use_stmt, stmt, var, this_maybe, equality); continue; @@ -4486,7 +4487,7 @@ pass_waccess::check_dangling_uses (tree var, tree decl, bool maybe /* = false */ basic_block use_bb = gimple_bb (use_stmt); basic_block clob_bb = gimple_bb (*pclob); - maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, clob_bb); + maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, clob_bb, use_bb); warn_invalid_pointer (var, use_stmt, *pclob, decl, maybe, false); }
On Wed, 27 Apr 2022, Richard Biener wrote: > On Wed, 27 Apr 2022, Jakub Jelinek wrote: > > > On Wed, Apr 27, 2022 at 09:43:59AM +0200, Richard Biener wrote: > > > but not any operations on the pointer value (compare, pointer-plus, > > > difference, masking, etc.). A simple-minded implementation would > > > then be > > > > > > if ((!gimple_vuse (use_stmt) && warn_dangling_pointer < 3) > > > || (maybe && ... > > > > That would consider as memory uses both cases where we actually dereference > > the pointer and where we store the pointer in some memory. > > But perhaps that would be the goal. > > I think that was the intention of the diagnostic, yes. I don't think > it's very useful to diagnose that you add 1 to a dangling pointer, > instead I hope the code will diagnose the dereference of the offsetted > dangling pointer instead (I think it does, though it might miss _1 = > &MEM[_2 + 4]; from a quick look, likewise _2 & ~3 (aligning a pointer) > and any tricks via uintptr casting). > > There's some testsuite fallout with using !gimple_vuse () because > when a diagnostic is suppressed we are _not_ tracking derived pointers. > > For c-c++-common/Wdangling-pointer.c at -O0 we diagnose > > void* warn_return_local_addr (void) > { > int *p = 0; > { > int a[] = { 1, 2, 3 }; > p = a; > } > > /* Unlike the above case, here the pointer is dangling when it's > used. */ > return p; // { dg-warning "using dangling pointer 'p' > to 'a'" "array" } > } > > _8 = p_6; // <-- here > > <bb3>: > return _8; > > but if we suppress that we do not add _8 to the pointers to check. > Even if we do add it we get a maybe diagnostic because the > post-dominator query uses reversed arguments (oops, we should probably > fix that independently). Fixing that results in > 'using a dangling pointer to 'a'' (instead of naming 'p') since _8 > is anonymous. > > When optimizing we fail to diagnose this case - we skip return stmts > because of -Wreturn-local-addr but when we do not warn about the > artificial SSA copy the diagnostic is lost. So that's the only > "intentional" diagnostic that's skipped by !gimple_vuse (). > > Still it requires too much changes to code I'm not familiar with > at this point. > > > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > > > index 879dbcc1e52..80b5119da7d 100644 > > > --- a/gcc/gimple-ssa-warn-access.cc > > > +++ b/gcc/gimple-ssa-warn-access.cc > > > @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple > > > *use_ > > > stmt, > > > return; > > > } > > > > > > - if ((maybe && warn_dangling_pointer < 2) > > > + if (equality > > > + || (maybe && warn_dangling_pointer < 2) > > > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > > > return; > > > > > > > This is fine too with the invoke.texi change and the testcases. > > So I'm leaning towards this to fix the P1 bug and see what other > cases people come up with in the real world. > > As said if not introducing =3 I'd not document this implementation > detail (as said above we miss some cases because we fail to follow > all pointer adjustments as well). > > I'm re-testing the variant below, not documenting the implementation > detail but fixing the post-dominator queries. Bootstrapped & tested on x86_64-unknown-linux-gnu. OK? Thanks, Richard. > Richard. > > From 098ab3298b273a56bafe978facdc512789a7628a Mon Sep 17 00:00:00 2001 > From: Richard Biener <rguenther@suse.de> > Date: Mon, 25 Apr 2022 10:46:16 +0200 > Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling > pointer diags > To: gcc-patches@gcc.gnu.org > > The following extends the equality compare dangling pointer diagnostics > suppression for uses following free or realloc to also cover those > following invalidation of auto variables via CLOBBERs. That avoids > diagnosing idioms like > > return std::find(std::begin(candidates), std::end(candidates), s) > != std::end(candidates); > > for auto candidates which are prone to forwarding of the final > comparison across the storage invalidation as then seen by the > late run access warning pass. > > 2022-04-25 Richard Biener <rguenther@suse.de> > > PR middle-end/104492 > * gimple-ssa-warn-access.cc > (pass_waccess::warn_invalid_pointer): Exclude equality compare > diagnostics for all kind of invalidations. > (pass_waccess::check_dangling_uses): Fix post-dominator query. > (pass_waccess::check_pointer_uses): Likewise. > --- > gcc/gimple-ssa-warn-access.cc | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index 879dbcc1e52..39aa8186de6 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt, > return; > } > > - if ((maybe && warn_dangling_pointer < 2) > + if (equality > + || (maybe && warn_dangling_pointer < 2) > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) > return; > > @@ -4241,7 +4242,7 @@ pass_waccess::check_pointer_uses (gimple *stmt, tree ptr, > basic_block use_bb = gimple_bb (use_stmt); > bool this_maybe > = (maybe > - || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, stmt_bb)); > + || !dominated_by_p (CDI_POST_DOMINATORS, stmt_bb, use_bb)); > warn_invalid_pointer (*use_p->use, use_stmt, stmt, var, > this_maybe, equality); > continue; > @@ -4486,7 +4487,7 @@ pass_waccess::check_dangling_uses (tree var, tree decl, bool maybe /* = false */ > > basic_block use_bb = gimple_bb (use_stmt); > basic_block clob_bb = gimple_bb (*pclob); > - maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, clob_bb); > + maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, clob_bb, use_bb); > warn_invalid_pointer (var, use_stmt, *pclob, decl, maybe, false); > } > >
On Wed, Apr 27, 2022 at 11:56:55AM +0200, Richard Biener wrote: > Bootstrapped & tested on x86_64-unknown-linux-gnu. > > OK? I think a testcase from the #c0 of the PR would be nice, but it can be added incrementally, so ok for trunk and unless somebody beats me to it, I'll try to reduce the testcase. With a fixed and unfixed compiler around it might be easier for reduction. Jakub
On Wed, 27 Apr 2022, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 11:56:55AM +0200, Richard Biener wrote: > > Bootstrapped & tested on x86_64-unknown-linux-gnu. > > > > OK? > > I think a testcase from the #c0 of the PR would be nice, but it can > be added incrementally, so ok for trunk and unless somebody beats me > to it, I'll try to reduce the testcase. With a fixed and unfixed compiler > around it might be easier for reduction. I did that but the reduction result did not resemble the same failure mode. I've failed to manually construct a testcase as well. Possibly a testcase using libstdc++ but less Qt internals might be possible. Thanks, Richard.
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index 879dbcc1e52..6c404f18db7 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -3896,13 +3896,13 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt, return; } + if ((equality && warn_use_after_free < 3) + || (maybe && warn_use_after_free < 2) + || warning_suppressed_p (use_stmt, OPT_Wuse_after_free)) + return; + if (is_gimple_call (inval_stmt)) { - if ((equality && warn_use_after_free < 3) - || (maybe && warn_use_after_free < 2) - || warning_suppressed_p (use_stmt, OPT_Wuse_after_free)) - return; - const tree inval_decl = gimple_call_fndecl (inval_stmt); if ((ref && warning_at (use_loc, OPT_Wuse_after_free, @@ -3923,10 +3923,6 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt, return; } - if ((maybe && warn_dangling_pointer < 2) - || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_)) - return; - if (DECL_NAME (var)) { if ((ref diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c index 20f11b227d6..11c939cb086 100644 --- a/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c +++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c @@ -356,7 +356,7 @@ NOIPA void warn_cond_if_else (int i) } else { - int b[] = { 3, 4 }; // { dg-message "'b' declared" "pr??????" { xfail *-*-* } } + int b[] = { 3, 4 }; // { dg-message "'b' declared" } sink (b); p = b; } @@ -365,7 +365,7 @@ NOIPA void warn_cond_if_else (int i) because after the first diagnostic the code suppresses subsequent ones for the same use. This needs to be fixed. */ sink (p); // { dg-warning "dangling pointer 'p' to 'a' may be used" } - // { dg-warning "dangling pointer 'p' to 'b' may be used" "pr??????" { xfail *-*-* } .-1 } + // { dg-warning "dangling pointer 'p' to 'b' may be used" "" { target *-*-* } .-1 } } diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer.c b/gcc/testsuite/c-c++-common/Wdangling-pointer.c index 0a18c3c8249..09e4036a4dd 100644 --- a/gcc/testsuite/c-c++-common/Wdangling-pointer.c +++ b/gcc/testsuite/c-c++-common/Wdangling-pointer.c @@ -370,7 +370,7 @@ void warn_cond_if_else (int i) } else { - int b[] = { 3, 4 }; // { dg-message "'b' declared" "note" { xfail *-*-* } } + int b[] = { 3, 4 }; // { dg-message "'b' declared" "note" } sink (b); p = b; } @@ -379,7 +379,7 @@ void warn_cond_if_else (int i) because after the first diagnostic the code suppresses subsequent ones for the same use. This needs to be fixed. */ sink (p); // { dg-warning "dangling pointer 'p' to 'a' may be used" } - // { dg-warning "dangling pointer 'p' to 'b' may be used" "pr??????" { xfail *-*-* } .-1 } + // { dg-warning "dangling pointer 'p' to 'b' may be used" "" { target *-*-* } .-1 } }