Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Dodji Seketeli <dodji@redhat.com>]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> There are various conversion related warnings that trigger on
>> potentially dangerous uses of NULL (or __null).  NULL is defined as a
>> macro in a system header, so calling warning or warning_at on a virtual
>> location of NULL yields no diagnostic.  So the test accompanying this
>> patch (as well as others), was failling when run with
>> -ftrack-macro-expansion.
>>
>> I think it's necessary to use the location of NULL that is in the main
>> source code (instead of, e.g, the spelling location that is in the
>> system header where the macro is defined) in those cases.  Note that for
>> __null, we don't have the issue.
>
> I like the idea.  However, you should write a separate function
> (get_null_ptr_cst_location?) that computes the location of the null
> pointer constant token and call it from where you need the location.

OK.  I have introduced such a new function and I gave it the slightly
more generic name expansion_point_location_if_in_system_header as I
suspect it can be useful for macros that are similar to NULL.  I haven't
spotted such macros (from test regressions) yet, though.

Here is the updated patch that passes bootstrap with
-ftrack-macro-expansion turned off; it also passes bootstrap with
-ftrack-macro-expansion turned on with the whole series of patches I
have locally on my tree.

gcc/
	* input.h (expansion_point_location_if_in_system_header): Declare
	new function.
	* input.c (expansion_point_location_if_in_system_header): Define it.
gcc/cp/

	* call.c (conversion_null_warnings): Use the new
	expansion_point_location_if_in_system_header.
	* cvt.c (build_expr_type_conversion): Likewise.
	* typeck.c (cp_build_binary_op): Likewise.

gcc/testsuite/

	* g++.dg/warn/Wconversion-null-2.C: Add testing for __null,
	alongside the previous testing for NULL.
---
 gcc/cp/call.c                                  |   16 ++++++++++-
 gcc/cp/cvt.c                                   |   16 ++++++++++-
 gcc/cp/typeck.c                                |   18 +++++++++++--
 gcc/input.c                                    |   14 ++++++++++
 gcc/input.h                                    |    1 +
 gcc/testsuite/g++.dg/warn/Wconversion-null-2.C |   31 +++++++++++++++++++++++-
 6 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f9a7f08..1dc57fc 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5598,12 +5598,24 @@ conversion_null_warnings (tree totype, tree expr, tree fn, int argnum)
   if (expr == null_node && TREE_CODE (totype) != BOOLEAN_TYPE
       && ARITHMETIC_TYPE_P (totype))
     {
+      /* The location of the warning here is most certainly the one for
+	 the token that represented null_node in the source code.
+	 That is either NULL or __null.  If it is NULL, then that
+	 macro is defined in a system header and, as a consequence,
+	 warning_at won't emit any diagnostic for it.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this error
+	 message is, rather than the point where the NULL macro is
+	 defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
       if (fn)
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "passing NULL to non-pointer argument %P of %qD",
 		    argnum, fn);
       else
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "converting to non-pointer type %qT from NULL", totype);
     }
 
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index 3dab372..8defc61 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1472,8 +1472,20 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
   if (expr == null_node
       && (desires & WANT_INT)
       && !(desires & WANT_NULL))
-    warning_at (input_location, OPT_Wconversion_null,
-		"converting NULL to non-pointer type");
+    {
+      /* The location of the warning here is the one for the
+	 (expansion of the) NULL macro, or for __null.  If it is for
+	 NULL, then, as that that macro is defined in a system header,
+	 warning_at won't emit any diagnostic.  In this case, we are
+	 going to resolve that location to the point in the source
+	 code where the expression that triggered this warning is,
+	 rather than the point where the NULL macro is defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wconversion_null,
+		  "converting NULL to non-pointer type");
+    }
 
   basetype = TREE_TYPE (expr);
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index fb2f1bc..f453536 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3838,9 +3838,21 @@ cp_build_binary_op (location_t location,
 	  || (!null_ptr_cst_p (orig_op1) 
 	      && !TYPE_PTR_P (type1) && !TYPE_PTR_TO_MEMBER_P (type1)))
       && (complain & tf_warning))
-    /* Some sort of arithmetic operation involving NULL was
-       performed.  */
-    warning (OPT_Wpointer_arith, "NULL used in arithmetic");
+    {
+      /* Some sort of arithmetic operation involving NULL was
+	 performed.  The location of the warning here is the one for
+	 the (expansion of the) NULL macro, or for __null.  If it is
+	 for NULL, then, as that that macro is defined in a system
+	 header, warning_at won't emit any diagnostic.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this warning
+	 is, rather than the point where the NULL macro is
+	 defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
+    }
 
   switch (code)
     {
diff --git a/gcc/input.c b/gcc/input.c
index 260be7e..75457a3 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -162,6 +162,20 @@ expand_location_to_spelling_point (source_location loc)
   return expand_location_1 (loc, /*expansion_piont_p=*/false);
 }
 
+/* If LOCATION is in a sytem header and if it's a virtual location for
+   a token coming from the expansion of a macro M, unwind it to the
+   location of the expansion point of M.  Otherwise, just return
+   LOCATION.  */
+
+source_location
+expansion_point_location_if_in_system_header (source_location location)
+{
+  if (in_system_header_at (location))
+    location = linemap_resolve_location (line_table, location,
+					 LRK_MACRO_EXPANSION_POINT,
+					 NULL);
+  return location;
+}
 
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
diff --git a/gcc/input.h b/gcc/input.h
index f755cdf..f588838 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -40,6 +40,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 extern expanded_location expand_location (source_location);
 extern const char * location_get_source_line(expanded_location xloc);
 extern expanded_location expand_location_to_spelling_point (source_location);
+extern source_location expansion_point_location_if_in_system_header (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
index dd498c1..a71551f 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
@@ -25,7 +25,7 @@ void l(long) {}
 template <>
 void l(long long) {}
 
-int main()
+void warn_for_NULL()
 {
   int i = NULL; // { dg-warning "" } converting NULL to non-pointer type
   float z = NULL; // { dg-warning "" } converting NULL to non-pointer type
@@ -47,3 +47,32 @@ int main()
   l(NULL);   // No warning: NULL is used to implicitly instantiate the template
   NULL && NULL; // No warning: converting NULL to bool is OK
 }
+
+int warn_for___null()
+{
+  int i = __null; // { dg-warning "" } converting __null to non-pointer type
+  float z = __null; // { dg-warning "" } converting __null to non-pointer type
+  int a[2];
+
+  i != __null; // { dg-warning "" } __null used in arithmetic
+  __null != z; // { dg-warning "" } __null used in arithmetic
+  k != __null; // No warning: decay conversion
+  __null != a; // Likewise.
+  -__null;     // { dg-warning "" } converting __null to non-pointer type
+  +__null;     // { dg-warning "" } converting __null to non-pointer type
+  ~__null;     // { dg-warning "" } converting __null to non-pointer type
+  a[__null] = 3; // { dg-warning "" } converting __null to non-pointer-type
+  i = __null;  // { dg-warning "" } converting __null to non-pointer type
+  z = __null;  // { dg-warning "" } converting __null to non-pointer type
+  k(__null);   // { dg-warning "" } converting __null to int
+  g(__null);   // { dg-warning "" } converting __null to int
+  h<__null>(); // No warning: __null bound to integer template parameter
+  l(__null);   // No warning: __null is used to implicitly instantiate the template
+  __null && __null; // No warning: converting NULL to bool is OK
+}
+
+int main()
+{
+  warn_for_NULL();
+  warn_for___null();
+}
-- 
1.7.6.5


-- 
		Dodji