Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law <law@redhat.com>]

On 07/23/2015 04:44 PM, Bernhard Reutner-Fischer wrote:
> On July 23, 2015 7:43:51 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>> On 07/22/2015 09:29 AM, Manuel López-Ibáñez wrote:
>>> While looking at PR c/16351, I noticed that all tests proposed for
>>> -Wnull-attribute
>>> (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be
>>> warned from the FEs by simply extending the existing Wnonnull.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu.
>>>
>>> OK?
>>>
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-07-22  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>>
>>>       PR c/16351
>>>       * doc/invoke.texi (Wnonnull): Document behavior for
>>>       returns_nonnull.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2015-07-22  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>>
>>>       PR c/16351
>>>       * c-c++-common/wnonnull-1.c: New test.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 2015-07-22  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>>
>>>       PR c/16351
>>>       * typeck.c (check_return_expr): Call maybe_warn_returns_nonnull.
>>>
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>> 2015-07-22  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>>
>>>       PR c/16351
>>>       * c-common.c (maybe_warn_returns_nonnull): New.
>>>       * c-common.h (maybe_warn_returns_nonnull): Declare.
>>>
>>> gcc/c/ChangeLog:
>>>
>>> 2015-07-22  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>>
>>>       PR c/16351
>>>       * c-typeck.c (c_finish_return): Call maybe_warn_returns_nonnull.
>> FWIW, we have the usual tension here between warning in the front-end
>> vs
>> warning after optimization and exploiting dataflow analysis.
>>
>> 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.
>>
>> However warning in the front-ends will tend to have more stable
>> diagnostics from release to release.
>>
>> Warning after optimization/analysis will tend to generate fewer false
>> positives and can pick up cases where the didn't explicitly appear in
>> the return statement, but had to be propagated in by the optimizers. Of
>>
>> course, these warnings are less stable release-to-release and require
>> the optimizers/analysis phases to be run.
>>
>> 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'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.
>>
>> So please keep 16351 open after committing.
>
> -W{no-,}{f,m}e IIRC was proposed before. Won't help https://gcc.gnu.org/PR55035 though where struct stores just escape too much -- AFAIU -- but still..
>
Things like uninitialized variable analysis are inherently going to 
cause false positives.  It's just a fact of life.

Looking at the reduced testcase in that PR, I'm pretty sure its a bogus 
reduction.

We could have N == 0 when we encounter the head of the first loop.  So 
no members of temp[] will be initialized.  We then have the call to 
bar() which could change the value of N to 1.  We then hit the second 
loop and read temp[0] which is uninitialized.  The warning for the 
reduced testcase is correct.

If the original source has the same overall structure (an unbound write 
between the key loops), then the warning is correct.  If the writes can 
be proven to not clobber the loop bounds (such that the two key loops 
iterate over the same number of elements), then we'd have a false 
positive warning (and a failure of jump threading to discover the 
unexecutable path).

Jeff