Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Bernhard Reutner-Fischer <rep dot dot dot nop at gmail dot com>]

On July 24, 2015 7:30:03 AM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>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.

Yes, the original, smallish test case in comment #4 is different, AFAICS.
>
>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.

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

AFAIU the write in the testcase in comment #4 does not clobber loop bounds, so I think the warning is wrong.

Thanks,