From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95025 invoked by alias); 24 Jul 2015 23:15:31 -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 95015 invoked by uid 89); 24 Jul 2015 23:15:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: paperclip.tbsaunde.org Received: from tbsaunde.org (HELO paperclip.tbsaunde.org) (66.228.47.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Jul 2015 23:15:29 +0000 Received: from tsaunders-iceball.corp.tor1.mozilla.com (unknown [66.207.208.102]) by paperclip.tbsaunde.org (Postfix) with ESMTPSA id 5040EC06B; Fri, 24 Jul 2015 23:15:26 +0000 (UTC) Date: Sat, 25 Jul 2015 00:01:00 -0000 From: Trevor Saunders To: Manuel =?iso-8859-1?B?TPNwZXotSWLh8WV6?= Cc: Jeff Law , Gcc Patch List , Jason Merrill , "Joseph S. Myers" Subject: Re: PR c/16351 Extend Wnonnull for returns_nonnull Message-ID: <20150724231521.GB29097@tsaunders-iceball.corp.tor1.mozilla.com> References: <55B127D7.3020202@redhat.com> <55B29270.1030206@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-07/txt/msg02132.txt.bz2 On Fri, Jul 24, 2015 at 11:55:23PM +0200, Manuel López-Ibáñez wrote: > On 24 July 2015 at 21:30, Jeff Law wrote: > > On 07/24/2015 07:45 AM, Manuel López-Ibáñez 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 macros & > > 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 :-) I'd actually expect reasonable cases to be fairly common. For example T * foobar (int x) { if (blah) { UNREACHABLE(); return NULL; } ... } You may want to return a value to make some random compiler happy or whatever, but obviously the code should never run, and you may not have a real value. Another case is Foo * bar() { #if SHOULD_USE_BAR ... #else return NULL; #endif } And somehow your program is setup so bar is only called when SHOULD_USE_BAR is defined. In that sort of case it may be convenient for the function bar to always be defined, but not for the type Foo to be defined in which case there is no real value for the function to return. > >> 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 ;-) maybe, but I'd bet that just means the tests aren't very extensive ;-) > >> 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 away > > those which are false positives. I consider exposing the non-trivial cases > > 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. On the other hand those warnings absolutely require non trivial analysis, so if you can share code between analyzing for warnings and for optimization that seems valuable. Similarly if you can share the code between languages that seems useful. Of course that is even more important if you want to catch things that only become visible with inlining. > For example, Clang is currently able to warn about the following case > without any optimization, while GCC cannot at any optimization level: So at some point we're just talking semantics of words, but I think you could make a credible argument that clang doesn't have a true -O0 mode. I'm no expert on clang internals, but I think the place they draw a line between what is a front end and what is a back is farther back than where gcc draws it, but that's basically just a guess. Trev > > int f(bool b) { > int n; > if (b) > n = 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; > ^ > = 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=35587 > > Cheers, > > Manuel.