* [PATCH] Warn when comparing nonnull arguments to NULL in a function.
@ 2015-09-09 21:51 Mark Wielaard
2015-09-09 22:02 ` Jeff Law
2015-09-15 3:43 ` Martin Sebor
0 siblings, 2 replies; 10+ messages in thread
From: Mark Wielaard @ 2015-09-09 21:51 UTC (permalink / raw)
To: gcc-patches; +Cc: Mark Wielaard
The following found 14 bugs in my code base. I think it is useful to
warn about such usage since they are bugsr. If the argument is marked
as nonnull then passing in a NULL argument will produce bad results
even if the code checks against NULL.
GCC might optimize such checks away so warn the user when the function
contains such comparisions.
nn.c: In function ‘foo’:
nn.c:6:27: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
void foo(void *bar) { if (!bar) abort(); }
^
gcc/c/ChangeLog
* c-typeck.c (build_binary_op): Check and warn when nonnull arg
parm against NULL.
gcc/cp/ChangeLog
* typeck.c (cp_build_binary_op): Check and warn when nonnull arg
parm against NULL.
gcc/testsuite/ChangeLog
* gcc.dg/nonnull-4.c: New test.
* g++.dg/warn/nonnull3.C: Likewise.
---
gcc/c/ChangeLog | 5 +++++
gcc/c/c-typeck.c | 10 ++++++++++
gcc/cp/ChangeLog | 5 +++++
gcc/cp/typeck.c | 10 ++++++++++
gcc/testsuite/ChangeLog | 5 +++++
gcc/testsuite/g++.dg/warn/nonnull3.C | 29 +++++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/nonnull-4.c | 28 ++++++++++++++++++++++++++++
7 files changed, 92 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/nonnull3.C
create mode 100644 gcc/testsuite/gcc.dg/nonnull-4.c
diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
index d7eeb2d..35ccdda 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * c-typeck.c (build_binary_op): Check and warn when nonnull arg
+ parm against NULL.
+
2015-09-09 Jakub Jelinek <jakub@redhat.com>
PR c/67501
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index dc22396..4108f27 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -10803,6 +10803,11 @@ build_binary_op (location_t location, enum tree_code code,
short_compare = 1;
else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1))
{
+ if (warn_nonnull
+ && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op0);
+
if (TREE_CODE (op0) == ADDR_EXPR
&& decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
{
@@ -10823,6 +10828,11 @@ build_binary_op (location_t location, enum tree_code code,
}
else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0))
{
+ if (warn_nonnull
+ && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op1);
+
if (TREE_CODE (op1) == ADDR_EXPR
&& decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
{
diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
index 515a1e8..7cf0064 100644
--- a/gcc/cp/ChangeLog
+++ b/gcc/cp/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * typeck.c (cp_build_binary_op): Check and warn when nonnull arg
+ parm against NULL.
+
2015-09-09 Jakub Jelinek <jakub@redhat.com>
PR c++/67504
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 388558c..482e42c 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -4438,6 +4438,11 @@ cp_build_binary_op (location_t location,
|| (code0 == POINTER_TYPE
&& TYPE_PTR_P (type1) && integer_zerop (op1)))
{
+ if (warn_nonnull
+ && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op0);
+
if (TYPE_PTR_P (type1))
result_type = composite_pointer_type (type0, type1, op0, op1,
CPO_COMPARISON, complain);
@@ -4477,6 +4482,11 @@ cp_build_binary_op (location_t location,
|| (code1 == POINTER_TYPE
&& TYPE_PTR_P (type0) && integer_zerop (op0)))
{
+ if (warn_nonnull
+ && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op1);
+
if (TYPE_PTR_P (type0))
result_type = composite_pointer_type (type0, type1, op0, op1,
CPO_COMPARISON, complain);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 360fe70..be4abd0 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * gcc.dg/nonnull-4.c: New test.
+ * g++.dg/warn/nonnull3.C: Likewise.
+
2015-09-09 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
* gcc.target/aarch64/mod_2.x: New file.
diff --git a/gcc/testsuite/g++.dg/warn/nonnull3.C b/gcc/testsuite/g++.dg/warn/nonnull3.C
new file mode 100644
index 0000000..8cad937
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/nonnull3.C
@@ -0,0 +1,29 @@
+/* Test for the bad usage of "nonnull" function attribute parms. */
+/* Same as C test gcc.dg/nonnull-4.c because checks are done in frontend. */
+/* */
+/* { dg-do compile } */
+/* { dg-options "-Wnonnull" } */
+
+#include <stddef.h>
+#include <stdlib.h>
+
+void foo(void *bar) __attribute__((nonnull(1)));
+
+void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
+
+extern int func (char *, char *, char *, char *) __attribute__((nonnull));
+
+int
+func (char *cp1, char *cp2, char *cp3, char *cp4)
+{
+ if (cp1) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 1;
+
+ if (cp2 == NULL) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 2;
+
+ if (NULL != cp3) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 3;
+
+ return (cp4 != 0) ? 0 : 1; /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+}
diff --git a/gcc/testsuite/gcc.dg/nonnull-4.c b/gcc/testsuite/gcc.dg/nonnull-4.c
new file mode 100644
index 0000000..12f9356
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/nonnull-4.c
@@ -0,0 +1,28 @@
+/* Test for the bad usage of "nonnull" function attribute parms. */
+/* */
+/* { dg-do compile } */
+/* { dg-options "-Wnonnull" } */
+
+#include <stddef.h>
+#include <stdlib.h>
+
+void foo(void *bar) __attribute__((nonnull(1)));
+
+void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
+
+extern int func (char *, char *, char *, char *) __attribute__((nonnull));
+
+int
+func (char *cp1, char *cp2, char *cp3, char *cp4)
+{
+ if (cp1) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 1;
+
+ if (cp2 == NULL) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 2;
+
+ if (NULL != cp3) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 3;
+
+ return (cp4 != 0) ? 0 : 1; /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+}
--
2.4.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-09 21:51 [PATCH] Warn when comparing nonnull arguments to NULL in a function Mark Wielaard
@ 2015-09-09 22:02 ` Jeff Law
2015-09-09 22:33 ` Jakub Jelinek
2015-09-15 3:43 ` Martin Sebor
1 sibling, 1 reply; 10+ messages in thread
From: Jeff Law @ 2015-09-09 22:02 UTC (permalink / raw)
To: Mark Wielaard, gcc-patches
On 09/09/2015 03:44 PM, Mark Wielaard wrote:
> The following found 14 bugs in my code base. I think it is useful to
> warn about such usage since they are bugsr. If the argument is marked
> as nonnull then passing in a NULL argument will produce bad results
> even if the code checks against NULL.
>
> GCC might optimize such checks away so warn the user when the function
> contains such comparisions.
>
> nn.c: In function ‘foo’:
> nn.c:6:27: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> void foo(void *bar) { if (!bar) abort(); }
> ^
> gcc/c/ChangeLog
>
> * c-typeck.c (build_binary_op): Check and warn when nonnull arg
> parm against NULL.
>
> gcc/cp/ChangeLog
>
> * typeck.c (cp_build_binary_op): Check and warn when nonnull arg
> parm against NULL.
>
> gcc/testsuite/ChangeLog
>
> * gcc.dg/nonnull-4.c: New test.
> * g++.dg/warn/nonnull3.C: Likewise.
Can you also upate the -Wnonnull documentation in invoke.texi to
indicate it also will warn if it discovers a non-null argument that is
compared against null?
With the doc fix and a bootstrap/regression test, this patch ought to be
fine.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-09 22:02 ` Jeff Law
@ 2015-09-09 22:33 ` Jakub Jelinek
2015-09-09 23:01 ` Mark Wielaard
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Jelinek @ 2015-09-09 22:33 UTC (permalink / raw)
To: Jeff Law; +Cc: Mark Wielaard, gcc-patches
On Wed, Sep 09, 2015 at 04:01:07PM -0600, Jeff Law wrote:
> > * gcc.dg/nonnull-4.c: New test.
> > * g++.dg/warn/nonnull3.C: Likewise.
If the tests are the same, perhaps stick just one test into
c-c++-common/nonnull-1.c instead? Also, all the "cp1 compared to NULL"
strings mention cp1, did you mean the second one to mention cp2 and so on?
> Can you also upate the -Wnonnull documentation in invoke.texi to indicate it
> also will warn if it discovers a non-null argument that is compared against
> null?
>
> With the doc fix and a bootstrap/regression test, this patch ought to be
> fine.
Jakub
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-09 22:33 ` Jakub Jelinek
@ 2015-09-09 23:01 ` Mark Wielaard
2015-09-14 19:26 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Mark Wielaard @ 2015-09-09 23:01 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 927 bytes --]
On Thu, 2015-09-10 at 00:03 +0200, Jakub Jelinek wrote:
> On Wed, Sep 09, 2015 at 04:01:07PM -0600, Jeff Law wrote:
> > > * gcc.dg/nonnull-4.c: New test.
> > > * g++.dg/warn/nonnull3.C: Likewise.
>
> If the tests are the same, perhaps stick just one test into
> c-c++-common/nonnull-1.c instead?
Yes, that would be better. The warnings should be exactly the same.
> Also, all the "cp1 compared to NULL"
> strings mention cp1, did you mean the second one to mention cp2 and so on?
Oops. copy/paste error indeed.
> > Can you also upate the -Wnonnull documentation in invoke.texi to indicate it
> > also will warn if it discovers a non-null argument that is compared against
> > null?
> >
> > With the doc fix and a bootstrap/regression test, this patch ought to be
> > fine.
Documentation added. bootstrap/regression test still running.
Updated patch attached.
Thanks,
Mark
[-- Attachment #2: Type: text/x-patch, Size: 6913 bytes --]
From d8d71393c2fde83769d00c2da766a2fa7955ecbb Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 9 Sep 2015 23:26:54 +0200
Subject: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
GCC might optimize such checks away so warn the user when the function
contains such comparisons.
nn.c: In function ‘foo’:
nn.c:6:27: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
void foo(void *bar) { if (!bar) abort(); }
^
gcc/ChangeLog
* doc/invoke.texi (Wnonnull): Also warns when comparing against NULL.
gcc/c/ChangeLog
* c-typeck.c (build_binary_op): Check and warn when nonnull arg
parm against NULL.
gcc/cp/ChangeLog
* typeck.c (cp_build_binary_op): Check and warn when nonnull arg
parm against NULL.
gcc/testsuite/ChangeLog
* c-c++-common/nonnull-1.c: New test.
---
gcc/ChangeLog | 4 ++++
gcc/c/ChangeLog | 5 +++++
gcc/c/c-typeck.c | 10 ++++++++++
gcc/cp/ChangeLog | 5 +++++
gcc/cp/typeck.c | 10 ++++++++++
gcc/doc/invoke.texi | 3 +++
gcc/testsuite/ChangeLog | 4 ++++
gcc/testsuite/c-c++-common/nonnull-1.c | 28 ++++++++++++++++++++++++++++
8 files changed, 69 insertions(+)
create mode 100644 gcc/testsuite/c-c++-common/nonnull-1.c
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 618bbe6..86038f5 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,7 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * doc/invoke.texi (Wnonnull): Also warns when comparing against NULL.
+
2015-09-09 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
* config/arm/arm.md (*subsi3_compare0): Rename to...
diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
index d7eeb2d..35ccdda 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * c-typeck.c (build_binary_op): Check and warn when nonnull arg
+ parm against NULL.
+
2015-09-09 Jakub Jelinek <jakub@redhat.com>
PR c/67501
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index dc22396..4108f27 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -10803,6 +10803,11 @@ build_binary_op (location_t location, enum tree_code code,
short_compare = 1;
else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1))
{
+ if (warn_nonnull
+ && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op0);
+
if (TREE_CODE (op0) == ADDR_EXPR
&& decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
{
@@ -10823,6 +10828,11 @@ build_binary_op (location_t location, enum tree_code code,
}
else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0))
{
+ if (warn_nonnull
+ && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op1);
+
if (TREE_CODE (op1) == ADDR_EXPR
&& decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
{
diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
index 515a1e8..7cf0064 100644
--- a/gcc/cp/ChangeLog
+++ b/gcc/cp/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * typeck.c (cp_build_binary_op): Check and warn when nonnull arg
+ parm against NULL.
+
2015-09-09 Jakub Jelinek <jakub@redhat.com>
PR c++/67504
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 388558c..482e42c 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -4438,6 +4438,11 @@ cp_build_binary_op (location_t location,
|| (code0 == POINTER_TYPE
&& TYPE_PTR_P (type1) && integer_zerop (op1)))
{
+ if (warn_nonnull
+ && TREE_CODE (op0) == PARM_DECL && nonnull_arg_p (op0))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op0);
+
if (TYPE_PTR_P (type1))
result_type = composite_pointer_type (type0, type1, op0, op1,
CPO_COMPARISON, complain);
@@ -4477,6 +4482,11 @@ cp_build_binary_op (location_t location,
|| (code1 == POINTER_TYPE
&& TYPE_PTR_P (type0) && integer_zerop (op0)))
{
+ if (warn_nonnull
+ && TREE_CODE (op1) == PARM_DECL && nonnull_arg_p (op1))
+ warning_at (location, OPT_Wnonnull,
+ "nonnull argument %qD compared to NULL", op1);
+
if (TYPE_PTR_P (type0))
result_type = composite_pointer_type (type0, type1, op0, op1,
CPO_COMPARISON, complain);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 76e5e29..dc171ec 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3720,6 +3720,9 @@ formats that may yield only a two-digit year.
Warn about passing a null pointer for arguments marked as
requiring a non-null value by the @code{nonnull} function attribute.
+Also warns when comparing an argument marked with the @code{nonnull}
+function attribute against null inside the function.
+
@option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}. It
can be disabled with the @option{-Wno-nonnull} option.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 360fe70..345caee 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2015-09-09 Mark Wielaard <mjw@redhat.com>
+
+ * c-c++-common/nonnull-1.c: New test.
+
2015-09-09 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
* gcc.target/aarch64/mod_2.x: New file.
diff --git a/gcc/testsuite/c-c++-common/nonnull-1.c b/gcc/testsuite/c-c++-common/nonnull-1.c
new file mode 100644
index 0000000..744c45f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/nonnull-1.c
@@ -0,0 +1,28 @@
+/* Test for the bad usage of "nonnull" function attribute parms. */
+/* */
+/* { dg-do compile } */
+/* { dg-options "-Wnonnull" } */
+
+#include <stddef.h>
+#include <stdlib.h>
+
+void foo(void *bar) __attribute__((nonnull(1)));
+
+void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
+
+extern int func (char *, char *, char *, char *) __attribute__((nonnull));
+
+int
+func (char *cp1, char *cp2, char *cp3, char *cp4)
+{
+ if (cp1) /* { dg-warning "nonnull argument" "cp1 compared to NULL" } */
+ return 1;
+
+ if (cp2 == NULL) /* { dg-warning "nonnull argument" "cp2 compared to NULL" } */
+ return 2;
+
+ if (NULL != cp3) /* { dg-warning "nonnull argument" "cp3 compared to NULL" } */
+ return 3;
+
+ return (cp4 != 0) ? 0 : 1; /* { dg-warning "nonnull argument" "cp4 compared to NULL" } */
+}
--
2.4.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-09 23:01 ` Mark Wielaard
@ 2015-09-14 19:26 ` Jeff Law
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-09-14 19:26 UTC (permalink / raw)
To: Mark Wielaard, Jakub Jelinek; +Cc: gcc-patches
On 09/09/2015 04:33 PM, Mark Wielaard wrote:
> On Thu, 2015-09-10 at 00:03 +0200, Jakub Jelinek wrote:
>> On Wed, Sep 09, 2015 at 04:01:07PM -0600, Jeff Law wrote:
>>>> * gcc.dg/nonnull-4.c: New test.
>>>> * g++.dg/warn/nonnull3.C: Likewise.
>>
>> If the tests are the same, perhaps stick just one test into
>> c-c++-common/nonnull-1.c instead?
>
> Yes, that would be better. The warnings should be exactly the same.
>
>> Also, all the "cp1 compared to NULL"
>> strings mention cp1, did you mean the second one to mention cp2 and so on?
>
> Oops. copy/paste error indeed.
>
>>> Can you also upate the -Wnonnull documentation in invoke.texi to indicate it
>>> also will warn if it discovers a non-null argument that is compared against
>>> null?
>>>
>>> With the doc fix and a bootstrap/regression test, this patch ought to be
>>> fine.
>
> Documentation added. bootstrap/regression test still running.
>
> Updated patch attached.
Assuming the bootstrap & regression test completed without errors, this
patch is fine for the trunk. Please install if you haven't done so already.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-09 21:51 [PATCH] Warn when comparing nonnull arguments to NULL in a function Mark Wielaard
2015-09-09 22:02 ` Jeff Law
@ 2015-09-15 3:43 ` Martin Sebor
2015-09-15 8:48 ` Mark Wielaard
1 sibling, 1 reply; 10+ messages in thread
From: Martin Sebor @ 2015-09-15 3:43 UTC (permalink / raw)
To: Mark Wielaard, gcc-patches
> +void foo(void *bar) __attribute__((nonnull(1)));
> +
> +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
This looks like a very useful enhancement. Since the change is limited
to build_binary_op in the two front ends I wonder if the warning also
issued for other expressions? For example, suppose I were to add to
function foo above the following:
bool is_null = bar;
would GCC issue a warning? The same question goes for other expressions
non-binary expressions, including:
bar ? f () : g ();
or in C++:
bool x = static_cast<bool>(bar);
If not, I would think issuing it would make the feature even more
useful (and the diagnostics more consistent).
Martin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-15 3:43 ` Martin Sebor
@ 2015-09-15 8:48 ` Mark Wielaard
2015-09-15 12:22 ` Manuel López-Ibáñez
2015-09-15 14:56 ` Martin Sebor
0 siblings, 2 replies; 10+ messages in thread
From: Mark Wielaard @ 2015-09-15 8:48 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches
On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> > +void foo(void *bar) __attribute__((nonnull(1)));
> > +
> > +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
>
> This looks like a very useful enhancement. Since the change is limited
> to build_binary_op in the two front ends I wonder if the warning also
> issued for other expressions? For example, suppose I were to add to
> function foo above the following:
>
> bool is_null = bar;
>
> would GCC issue a warning? The same question goes for other expressions
> non-binary expressions, including:
>
> bar ? f () : g ();
Yes, it will:
nt.c: In function ‘tf’:
nt.c:9:3: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
bool is_null = bar;
^
nt.c:14:7: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
bar ? f () : g ();
^
> or in C++:
>
> bool x = static_cast<bool>(bar);
Likewise for the g++ frontend:
nt.c: In function ‘bool tf(void*)’:
nt.c:12:18: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
bool is_null = bar;
^
nt.c:14:19: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
bar ? f () : g ();
^
nt.c:16:33: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
bool x = static_cast<bool>(bar);
^
Although I now notice they differ on the placement of the carrot.
Maybe the location passed into the warning is not correct/ideal?
Cheers,
Mark
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-15 8:48 ` Mark Wielaard
@ 2015-09-15 12:22 ` Manuel López-Ibáñez
2015-09-15 16:27 ` Mark Wielaard
2015-09-15 14:56 ` Martin Sebor
1 sibling, 1 reply; 10+ messages in thread
From: Manuel López-Ibáñez @ 2015-09-15 12:22 UTC (permalink / raw)
To: Mark Wielaard, Martin Sebor; +Cc: GCC Patches
On 15/09/15 10:32, Mark Wielaard wrote:
> On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> Although I now notice they differ on the placement of the carrot.
> Maybe the location passed into the warning is not correct/ideal?
The caret is placed at the location given by expand_location(loc), with loc
being the location passed to warning_at(). As far as I am aware, there are no
bugs on that. If the caret is wrong, it is definitely because the location
passed is the wrong one. You need to find the correct one (which may appear up
in the call stack and may need to be passed down) and pass that one instead.
You can test the location with { dg-warning "18:nonnull argument" } where 18 is
the column number expected. I wish it was used more frequently, in particular
in those cases where we have the perfect location and it would be pity to regress.
Cheers,
Manuel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-15 8:48 ` Mark Wielaard
2015-09-15 12:22 ` Manuel López-Ibáñez
@ 2015-09-15 14:56 ` Martin Sebor
1 sibling, 0 replies; 10+ messages in thread
From: Martin Sebor @ 2015-09-15 14:56 UTC (permalink / raw)
To: Mark Wielaard; +Cc: gcc-patches
On 09/15/2015 02:32 AM, Mark Wielaard wrote:
> On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
>>> +void foo(void *bar) __attribute__((nonnull(1)));
>>> +
>>> +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
>>
>> This looks like a very useful enhancement. Since the change is limited
>> to build_binary_op in the two front ends I wonder if the warning also
>> issued for other expressions? For example, suppose I were to add to
>> function foo above the following:
>>
>> bool is_null = bar;
>>
>> would GCC issue a warning? The same question goes for other expressions
>> non-binary expressions, including:
>>
>> bar ? f () : g ();
>
> Yes, it will:
>
> nt.c: In function ‘tf’:
> nt.c:9:3: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> bool is_null = bar;
> ^
>
> nt.c:14:7: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> bar ? f () : g ();
> ^
>
>> or in C++:
>>
>> bool x = static_cast<bool>(bar);
>
> Likewise for the g++ frontend:
Great!
>
> nt.c: In function ‘bool tf(void*)’:
> nt.c:12:18: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> bool is_null = bar;
> ^
> nt.c:14:19: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> bar ? f () : g ();
> ^
IME, the C++ diagnostics aren't entirely consistent in where they
put the location but this one seems worse than I would expect. It
should point at bar.
OTOH, here are a couple of examples of the inconsistency, one with
the same problem as yours, so it's probably not something you did:
$ cat t.c && g++ -c t.c
int bar (void *p) {
return p < 0.0 ? 0 : 1;
struct A { } a;
return a ? 0 : 1;
}
t.c: In function ‘int bar(void*)’:
t.c:2:16: error: invalid operands of types ‘void*’ and ‘double’ to
binary ‘operator<’
return p < 0.0 ? 0 : 1;
^
t.c:4:20: error: could not convert ‘a’ from ‘bar(void*)::A’ to ‘bool’
return a ? 0 : 1;
^
> nt.c:16:33: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
> bool x = static_cast<bool>(bar);
> ^
>
> Although I now notice they differ on the placement of the carrot.
> Maybe the location passed into the warning is not correct/ideal?
I noticed the same problem in other g++ diagnostics. For instance,
in the messages below, the column is that of the closing parenthesis
rather than that of the operand:
$ cat t.c && g++ -c t.c
int foo (int, int);
int bar (void *p) {
foo (p, 0);
return static_cast<int>(p);
}
t.c: In function ‘int bar(void*)’:
t.c:4:14: error: invalid conversion from ‘void*’ to ‘int’ [-fpermissive]
foo (p, 0);
^
t.c:1:5: note: initializing argument 1 of ‘int foo(int, int)’
int foo (int, int);
^
t.c:5:30: error: invalid static_cast from type ‘void*’ to type ‘int’
return static_cast<int>(p);
^
Martin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Warn when comparing nonnull arguments to NULL in a function.
2015-09-15 12:22 ` Manuel López-Ibáñez
@ 2015-09-15 16:27 ` Mark Wielaard
0 siblings, 0 replies; 10+ messages in thread
From: Mark Wielaard @ 2015-09-15 16:27 UTC (permalink / raw)
To: Manuel López-Ibáñez; +Cc: Martin Sebor, GCC Patches
On Tue, 2015-09-15 at 14:21 +0200, Manuel López-Ibáñez wrote:
> On 15/09/15 10:32, Mark Wielaard wrote:
> > On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> > Although I now notice they differ on the placement of the carrot.
> > Maybe the location passed into the warning is not correct/ideal?
>
> The caret is placed at the location given by expand_location(loc), with loc
> being the location passed to warning_at(). As far as I am aware, there are no
> bugs on that. If the caret is wrong, it is definitely because the location
> passed is the wrong one. You need to find the correct one (which may appear up
> in the call stack and may need to be passed down) and pass that one instead.
>
> You can test the location with { dg-warning "18:nonnull argument" } where 18 is
> the column number expected. I wish it was used more frequently, in particular
> in those cases where we have the perfect location and it would be pity to regress.
As Marin pointed out, this is not a new issue. Since build_binary_op
effectively only has one (input) location (and possibly the
DECL_SOURCE_LOCATIONS of the expressions, which are irrelevant in this
case) it is hard to get the location for the diagnostic precise. We
really need the locations of the expressions, not just the input
location of the binary op (which already seems a little inconsistent).
As you say we would have to audit each call to build_binary_op and
somehow pass through the location of op1 and op2 (if we actually have
them in the first place). Given the large number of ways build_binary_op
is called this doesn't seem feasible.
The real solution is probably something like David's "Add source-ranges
for trees" https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00739.html
David mentions several alternatives. I am not sure which one would be
the most acceptable.
Cheers,
Mark
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-09-15 16:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-09 21:51 [PATCH] Warn when comparing nonnull arguments to NULL in a function Mark Wielaard
2015-09-09 22:02 ` Jeff Law
2015-09-09 22:33 ` Jakub Jelinek
2015-09-09 23:01 ` Mark Wielaard
2015-09-14 19:26 ` Jeff Law
2015-09-15 3:43 ` Martin Sebor
2015-09-15 8:48 ` Mark Wielaard
2015-09-15 12:22 ` Manuel López-Ibáñez
2015-09-15 16:27 ` Mark Wielaard
2015-09-15 14:56 ` Martin Sebor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).