From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84181 invoked by alias); 24 Jul 2015 21:56:08 -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 84171 invoked by uid 89); 24 Jul 2015 21:56:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f171.google.com Received: from mail-wi0-f171.google.com (HELO mail-wi0-f171.google.com) (209.85.212.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 24 Jul 2015 21:56:06 +0000 Received: by wibud3 with SMTP id ud3so44989446wib.0 for ; Fri, 24 Jul 2015 14:56:03 -0700 (PDT) X-Received: by 10.194.201.71 with SMTP id jy7mr32108769wjc.93.1437774963047; Fri, 24 Jul 2015 14:56:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.188.139 with HTTP; Fri, 24 Jul 2015 14:55:23 -0700 (PDT) In-Reply-To: <55B29270.1030206@redhat.com> References: <55B127D7.3020202@redhat.com> <55B29270.1030206@redhat.com> From: =?UTF-8?B?TWFudWVsIEzDs3Blei1JYsOhw7Fleg==?= Date: Fri, 24 Jul 2015 22:17:00 -0000 Message-ID: Subject: Re: PR c/16351 Extend Wnonnull for returns_nonnull To: Jeff Law Cc: Gcc Patch List , Jason Merrill , "Joseph S. Myers" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2015-07/txt/msg02124.txt.bz2 On 24 July 2015 at 21:30, Jeff Law wrote: > On 07/24/2015 07:45 AM, Manuel L=C3=B3pez-Ib=C3=A1=C3=B1ez wrote: >> >> On 23 July 2015 at 19:43, Jeff Law wrote: >>> >>> Warning in the front-ends like this can generate false positives (such = as >>> a >>> NULL return in an unreachable path and miss cases where the NULL has to >>> be >>> propagated into the return by later optimizations. >> >> >> False positives (for the warning as proposed right now) would be >> strange, since it would mean that a returns_nonnull function returns >> an explicit NULL in some code-path that is not meant to be executed. >> That sounds like a bug waiting to happen. > > Depends on how you choose to look at things. It's quite common via macro= s & > such to have unexecutable/unreachable paths. Whether or not to warn about > something found on such a path is a matter of personal preference. I think it is also a matter of the particular warning and on the balance of true positives vs. false positives in typical code-bases. In this case, returning NULL in any code-path from a returns_nonnull function, even if the path is currently unreachable via some macro configuration, seems a bad idea. Of course, I'm open to be proven wrong :-) >> Moreover, this warning works in exactly the same cases as >> __attribute__((nonnull)) does for function arguments, and so far those >> haven't been a source of false positives. > > I'm sure I could write one ;-) And given that a FE based version of this > will only catch explicit NULLs in argument lists, I consider it so weak as > to be virtually useless. Well, it is catching exactly all the cases that you were testing for in your original -Wnull-attribute patch ;-) >> I'm very much in favour of this, but only for things that cannot >> reliably be warned from the FE. Clang has shown that it is possible to >> improve much more the accuracy of warnings in the FE and still compile >> faster than GCC by performing some kind of fast CCP (and VRP?) in the >> FE (or make the CCP and VRP passes occur earlier and even without >> optimization): > > And my assertion is that for things like we're discussing, you need the > analysis & optimizations both to expose the non-trivial cases and prune a= way > those which are false positives. I consider exposing the non-trivial cas= es > and pruning away false positives the most important aspect of this kind of > work. Based on my experience, I'm not convinced that moving warnings to the middle-end is a good idea. The middle-end does a very poor job keeping sensible locations when doing transformations and it will not only introduce false positives, it will also remove true positives. The diagnostics often refer to the wrong variable or code that is not what the user originally wrote, which makes very hard to understand the problem. One only has to read all the reports we have about -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other middle-end warnings. For example, Clang is currently able to warn about the following case without any optimization, while GCC cannot at any optimization level: int f(bool b) { int n; if (b) n =3D 1; return n; } sometimes-uninit.cpp:3:7: warning: variable 'n' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (b) ^ sometimes-uninit.cpp:5:10: note: uninitialized use occurs here return n; ^ sometimes-uninit.cpp:3:3: note: remove the 'if' if its condition is always = true if (b) ^~~~~~ sometimes-uninit.cpp:2:8: note: initialize the variable 'n' to silence this warning int n; ^ =3D 0 Another example is -Warray-bounds, for which the warnings from Clang are not only more precise: deadcode.cpp:7:5: warning: array index 44 is past the end of the array (which contains 42 elements) [-Warray-bounds] f(arr[N]); ^ ~ than the same warning from GCC : deadcode.cpp:7:4: warning: array subscript is above array bounds [-Warray-bounds] f(arr[N]); ^ but they work even without -O2: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D35587 Cheers, Manuel.