Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion
Gabriel Dos Reis g...@integrable-solutions.net writes:
On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli do...@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
Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion
On Wed, Apr 25, 2012 at 8:42 AM, Dodji Seketeli do...@redhat.com wrote: Gabriel Dos Reis g...@integrable-solutions.net writes: On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli do...@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. Thanks. But a point of the suggestion was that we won't need the same comment/explanation duplicated over at least 3 places. Just one. All those three places deal exactly with one instance: null pointer constant. That deserves a function in and of itself, which is documented by the duplicated comments. Please make that change. Everything else is OK. thanks.
Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion
Gabriel Dos Reis g...@integrable-solutions.net writes:
Thanks. But a point of the suggestion was that we won't need the
same comment/explanation duplicated over at least 3 places. Just
one. All those three places deal exactly with one instance: null
pointer constant. That deserves a function in and of itself, which
is documented by the duplicated comments.
Please make that change. Everything else is OK. thanks.
I am sorry for the round trips. Please find below a patch udpated
accordingly.
I am bootstrapping the whole patch set, but the impacted files of this
patch have built fine so far.
Thanks.
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 |7 -
gcc/cp/cvt.c |9 +-
gcc/cp/typeck.c|9 --
gcc/input.c| 20 +++
gcc/input.h|1 +
gcc/testsuite/g++.dg/warn/Wconversion-null-2.C | 31 +++-
6 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f9a7f08..85e45c2 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5598,12 +5598,15 @@ conversion_null_warnings (tree totype, tree expr, tree
fn, int argnum)
if (expr == null_node TREE_CODE (totype) != BOOLEAN_TYPE
ARITHMETIC_TYPE_P (totype))
{
+ 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..49ba38a 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1472,8 +1472,13 @@ 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);
+{
+ 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..52d264b 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3838,9 +3838,12 @@ 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);
+{
+ 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..5f14489 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -162,6 +162,26 @@ 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.
+
+ This is used for instance when we want to emit diagnostics about a
+ token that is located in a macro that is itself defined in a system
+ header -- e.g for the NULL macro. In that case, if LOCATION is
+ passed to diagnostics emitting functions like warning_at as is, no
+ diagnostic won't be emitted. */
+
+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
Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion
OK, thanks! -- Gaby
Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion
On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli do...@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.
@@ -5538,12 +5538,26 @@ conversion_null_warnings (tree totype, tree expr,
tree fn, int argnum)
if (expr == null_node TREE_CODE (totype) != BOOLEAN_TYPE
ARITHMETIC_TYPE_P (totype))
{
+ source_location loc = input_location;
+ /* 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. */
+ if (in_system_header_at (input_location))
+ loc = linemap_resolve_location (line_table, loc,
+ LRK_MACRO_EXPANSION_POINT,
+ NULL);
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 c411a47..73bdf71 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1468,8 +1468,22 @@ 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);
+ {
+ source_location loc = input_location;
+ /* 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. */
+ if (in_system_header_at (loc))
+ loc = linemap_resolve_location (line_table, loc,
+ LRK_MACRO_EXPANSION_POINT,
+ NULL);
+ 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 d2ed940..d096f1e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3798,9 +3798,23 @@ 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);
+ {
+ source_location loc = input_location;
+ /* 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. */
+ if (in_system_header_at (loc))
+ loc = linemap_resolve_location (line_table, loc,
+ LRK_MACRO_EXPANSION_POINT,
+ NULL);
+ warning_at (loc,
