PR c/16351 Extend Wnonnull for returns_nonnull

PR c/16351 Extend Wnonnull for returns_nonnull

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

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.

[-- Attachment #2: wnonnull-1.diff --]
[-- Type: text/plain, Size: 4780 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 225868)
+++ gcc/doc/invoke.texi	(working copy)
@@ -3709,11 +3709,13 @@ formats that may yield only a two-digit 
 
 @item -Wnonnull
 @opindex Wnonnull
 @opindex Wno-nonnull
 Warn about passing a null pointer for arguments marked as
-requiring a non-null value by the @code{nonnull} function attribute.
+requiring a non-null value by the @code{nonnull} function attribute
+or returning a null pointer from a function declared with the attribute
+@code{returns_nonnull}.
 
 @option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}.  It
 can be disabled with the @option{-Wno-nonnull} option.
 
 @item -Winit-self @r{(C, C++, Objective-C and Objective-C++ only)}
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 225868)
+++ gcc/c-family/c-common.c	(working copy)
@@ -9508,10 +9508,22 @@ check_nonnull_arg (void * ARG_UNUSED (ct
   if (integer_zerop (param))
     warning (OPT_Wnonnull, "null argument where non-null required "
 	     "(argument %lu)", (unsigned long) param_num);
 }
 
+/* Possibly warn if RETVAL is a null pointer and FNDECL is declared
+   with attribute returns_nonnull.  LOC is the location of RETVAL.  */
+
+void
+maybe_warn_returns_nonnull (location_t loc, tree fndecl, tree retval)
+{
+  if (integer_zerop (retval)
+      && lookup_attribute ("returns_nonnull",
+			   TYPE_ATTRIBUTES (TREE_TYPE (fndecl))))
+    warning_at (loc, OPT_Wnonnull, "null return value where non-null required");
+}
+
 /* Helper for nonnull attribute handling; fetch the operand number
    from the attribute argument list.  */
 
 static bool
 get_nonnull_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp)
Index: gcc/c-family/c-common.h
===================================================================
--- gcc/c-family/c-common.h	(revision 225868)
+++ gcc/c-family/c-common.h	(working copy)
@@ -1049,10 +1049,11 @@ extern void do_warn_double_promotion (tr
 extern void set_underlying_type (tree);
 extern void record_types_used_by_current_var_decl (tree);
 extern void record_locally_defined_typedef (tree);
 extern void maybe_record_typedef_use (tree);
 extern void maybe_warn_unused_local_typedefs (void);
+extern void maybe_warn_returns_nonnull (location_t, tree, tree);
 extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree);
 extern vec<tree, va_gc> *make_tree_vector (void);
 extern void release_tree_vector (vec<tree, va_gc> *);
 extern vec<tree, va_gc> *make_tree_vector_single (tree);
 extern vec<tree, va_gc> *make_tree_vector_from_list (tree);
Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 225868)
+++ gcc/c/c-typeck.c	(working copy)
@@ -9372,10 +9372,11 @@ c_finish_return (location_t loc, tree re
 	{
 	  semantic_type = TREE_TYPE (retval);
 	  retval = TREE_OPERAND (retval, 0);
 	}
       retval = c_fully_fold (retval, false, NULL);
+      maybe_warn_returns_nonnull (loc, current_function_decl, retval);
       if (semantic_type)
 	retval = build1 (EXCESS_PRECISION_EXPR, semantic_type, retval);
     }
 
   if (!retval)
Index: gcc/testsuite/c-c++-common/wnonnull-1.c
===================================================================
--- gcc/testsuite/c-c++-common/wnonnull-1.c	(revision 0)
+++ gcc/testsuite/c-c++-common/wnonnull-1.c	(revision 0)
@@ -0,0 +1,42 @@
+/* { dg-do compile } */ 
+/* { dg-options "-Wnonnull" } */
+
+
+extern void foo(void *) __attribute__ ((__nonnull__ (1)));
+
+int z;
+int y;
+
+void
+com (int a)
+{
+  foo (a == 42 ? &z  : (void *) 0); /* { dg-warning "null" } */
+}
+
+void
+bar (void)
+{
+  foo ((void *)0); /* { dg-warning "null" } */
+}
+
+int * foo_r(int a) __attribute__((returns_nonnull));
+int * bar_r(void) __attribute__((returns_nonnull));
+
+int *
+foo_r(int a)
+{
+  switch (a)
+    {
+      case 0:
+        return &z;
+      default:
+        return (int *)0; /* { dg-warning "null" } */
+    }
+}
+
+int *
+bar_r (void)
+{
+  return 0;		/* { dg-warning "null" } */
+}
+
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 225868)
+++ gcc/cp/typeck.c	(working copy)
@@ -8702,10 +8702,12 @@ check_return_expr (tree retval, bool *no
   /* We don't need to do any conversions when there's nothing being
      returned.  */
   if (!retval)
     return NULL_TREE;
 
+  maybe_warn_returns_nonnull (input_location, current_function_decl, retval);
+
   /* Do any required conversions.  */
   if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl))
     /* No conversions are required.  */
     ;
   else

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

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.

Jeff

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Bernhard Reutner-Fischer]

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

Thanks,

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

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

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Bernhard Reutner-Fischer]

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,

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 02:06 AM, Bernhard Reutner-Fischer wrote:
>>
>> 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.
Agreed.  I should have been a bit more explicit.  The test in c#5 is not 
a valid reduction.  I glanced at the testcase in c#4 and I believe it is 
a valid reduction.

I've reflected those findings as well as postulated on how the case from 
c#4 could be fixed in the BZ.

Jeff

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Manuel López-Ibáñez]

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.

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'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):

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?

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

Cheers,

Manuel.

Re: 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

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Manuel López-Ibáñez]

On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote:
> 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.

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

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

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Patrick Palka]

On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote:
>> 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.
>
> 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 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.
>
> 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 = 1;
>   return n;
> }

Is there a PR for this particular test case?  I am interested in
improving the uninit analysis for gcc 6 so this potentially seems up
my alley.

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Manuel López-Ibáñez]

On 25 July 2015 at 00:17, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
> Is there a PR for this particular test case?  I am interested in
> improving the uninit analysis for gcc 6 so this potentially seems up
> my alley.

We do not warn because of the infamous PR18501 (probably the
-Wuninitialized bug with the highest number of duplicates), where CPP
removes the default SSA definition of n and simply returns 1
unconditionally. But fixing PR18501 may not be necessary to detect
this case (Clang does it before doing any optimization).

There are other cases that would be better warned from the FE:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808

Cheers,

Manuel.



.

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 04:41 PM, Manuel López-Ibáñez wrote:
> On 25 July 2015 at 00:17, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>> Is there a PR for this particular test case?  I am interested in
>> improving the uninit analysis for gcc 6 so this potentially seems up
>> my alley.
>
> We do not warn because of the infamous PR18501 (probably the
> -Wuninitialized bug with the highest number of duplicates), where CPP
> removes the default SSA definition of n and simply returns 1
> unconditionally. But fixing PR18501 may not be necessary to detect
> this case (Clang does it before doing any optimization).
You don't have to move the warning to the FE to fix this bug. Go back to 
my proposal from several years ago which has a warning analysis phase at 
the start and the end of the optimization pipeline. It can catch this 
because it gets a chance to analyze the code prior to the generate PHI 
elimination.

>
> There are other cases that would be better warned from the FE:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808
I haven't looked at any of these recently.  But fundamentally you don't 
have to move the warning into the front-end, you just have to move it to 
the start of the optimization pipeline.

jeff

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 04:17 PM, Patrick Palka wrote:
> On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote:
>>> 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.
>>
>> 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 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.
>>
>> 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 = 1;
>>    return n;
>> }
>
> Is there a PR for this particular test case?  I am interested in
> improving the uninit analysis for gcc 6 so this potentially seems up
> my alley.
To fix this you have to stop the reduction of degenerate PHIs when the 
RHS has a single real value and one or more undefined values.   One of 
the advantages of the two pass scheme I suggested years ago is it would 
allow us to detect this before the optimizers collapsed the degenerate PHI.

jeff
>

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Trevor Saunders]

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 <law@redhat.com> wrote:
> > 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.
> 
> 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.

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Manuel López-Ibáñez]

On 25 July 2015 at 01:15, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> I'd actually expect reasonable cases to be fairly common.  For example

I added it to -Wall, I guess we'll see... not sure how many users of
returns_nonnull are out there.

In all your cases there is no reason to prefer NULL rather than, say,
(T*)0xdeadbeef.

Moreover, moving the warning to the ME does not guarantee that no
false positives will be emitted either. It also does not guarantee
that no false positives will be introduced by optimization that did
not exist in the user code. And optimizations may remove trivial true
positives, which seems even worse.

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

I think they make a distinction between dataflow analysis and code
transformation. Clang does dataflow analysis on the AST without
changing the code (I think it can even do interprocedural dataflow),
but it leaves the succession of analysis and transformation passes to
LLVM, which does not do warnings. I do not know the precise details of
how all this works, but it seems to work better and faster than what
GCC does right now for Wuninitialized.

Cheers,

Manuel.

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 05:15 PM, Trevor Saunders wrote:
>
>>>> 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 ;-)
Exactly.  Frankly extending them to cases where they're missed by a 
front-end only solution, but caught after analysis is fairly trivial. 
Pull the constant out into a temporary.  Viola, the front end only 
solution fails miserably.  It's so trivial it didn't seem worth it, but 
with a FE solution in place, it's probably worth it now.


>
>   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.
And that's probably a reasonable way to look at the situation.  We can 
easily put the line at the GENERIC->GIMPLE transition, pay the 
compile-time penalty and generate warnings at that point regardless of 
-O0 or -O2.  We've chosen not to do that so far.  It's a piece of the 
discussion I don't much care about.  If that's the way folks want to go, 
it shouldn't be hard to dust off the work I did a decade or so ago and 
make it work in the current tree.  But I'm not terribly inclined to do 
it myself anymore -- primarily because I weary of arguing one way or the 
other.


Jeff

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Bernhard Reutner-Fischer]

On July 25, 2015 1:15:21 AM GMT+02:00, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:

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

Which just proves that cxx is just too broken^wcomplicated to grok for any human being or compiler, for that matter |)

Cheers,

Re: PR c/16351 Extend Wnonnull for returns_nonnull

[Jeff Law]

On 07/24/2015 03:55 PM, Manuel López-Ibáñez wrote:
>
> 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 :-)
Again, this is a matter of personal preference and I think we come from 
two totally different mindsets when  it comes to this class of warnings.

>
>>> 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 ;-)
Which is an argument that we should have more complex testcases -- 
there's certainly real world code where this kind of stuff is an issue.


>
>>> 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.
Again, I think we come at this from fundamentally different viewpoints. 
     I'm willing to compromise on locations and I think we have a 
different viewpoint of what a true positive is.  If it's on an 
unreachable path, then I consider warning about it a false positive.

Those are based on decades of writing the analysis, then using the 
result then being horrified when I see things that are missed because a 
particular warning is so weak that it's nearly useless (-Warray-bounds 
comes to mind).

>
> 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 = 1;
>    return n;
> }
ANd this is a known failing in GCC.  Interestingly enough I believe my 
proposed revamping of how we handle uninitialized warnings would catch 
this IIRC.

>> Another example is -Warray-bounds, for which the warnings from Clang
> are not only more precise:
Our implementation of -Warray-bounds sucks because it only warns when we 
have a range that collapses down to a singularity that feeds into an 
array reference.  It does absolutely no may-be-out-of-bounds analysis. 
For that reason I consider -Warray-bounds as it stands today 
fundamentally broken.

>
> but they work even without -O2:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35587
I don't really care about that.  Others do, I don't.  But this is 
something folks can fix if the consensus is that we want these warnings 
with the optimizer off.  It's a trade-off, just like just about 
everything in this space.


jeff