PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote:
> On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> 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.


>
> 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.

In fact, when I wrote the patches around 16351 I certainly did find 
cases where NULL values were flowing into places where they shouldn't. 
Some were fairly complex and we fixed them -- there's no way the trivial 
front-end stuff would have found them.


>
>> I've always preferred exploiting optimization and analysis to both reduce
>> false positives and expose the non-trivial which show up via optimizations.
>> But I also understand that's simply my preference and that others have a
>> different preference.
>
> 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.

>
> https://gcc.gnu.org/PR38470 is just one example that would be very
> much improved by some trivial VRP.
>
>> I'll tentatively approve for the trunk, but I think we still want warnings
>> after optimization/analysis.  Which will likely lead to a scheme like I
>> proposed many years for uninitialized variables where we have multiple
>> modes.  One warns in the front-end like your implemention does, the other
>> defers the warning until after analysis & optimization.
>
> Isn't this what we already have with -Wuninitialized and -Wmaybe-uninitialized?
I'm pretty sure this is different than what I've proposed (and even 
coded up) in the past.

Essentially we run the uninitialized warning analysis twice, 
saving/comparing the results.  One is run just after the into-ssa phase 
(from which you can get the release-to-release stable warnings), the 
other as we're done with the optimization pipeline (which allows more 
precision).  You can even do things like "XYZ was used uninitialized, 
but all those uses were removed by the optimizers".





>
> It would be OK to get warnings for returning NULL implicitly (either
> by propagation, inlining, VRP). I plan to leave 16351 open for that
> (or open a new PR). However, in this case, I also think it makes sense
> to follow what -Wnonnull already does and always warn for any explicit
> "return NULL", even if the code as currently written is never
> executed. The two things are not incompatible.
>
>> So please keep 16351 open after committing.
>
> Of course, there is also
> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01860.html pending
Yes, it's on my list.

jeff