Re: [PATCH] Add -Wabsolute-value
On 9/4/18 3:08 AM, Martin Jambor wrote:
> Hi,
>
> On Fri, Aug 31 2018, Joseph Myers wrote:
>> On Fri, 31 Aug 2018, Martin Jambor wrote:
>>
>>> diff --git a/gcc/common.opt b/gcc/common.opt
>>> index ebc3ef43ce2..2950760fb2a 100644
>>> --- a/gcc/common.opt
>>> +++ b/gcc/common.opt
>>> @@ -815,6 +815,10 @@ Wvector-operation-performance
>>> Common Var(warn_vector_operation_performance) Warning
>>> Warn when a vector operation is compiled outside the SIMD.
>>>
>>> +Wabsolute-value
>>> +Common Var(warn_absolute_value) Warning EnabledBy(Wextra)
>>> +Warn on suspicious calls of standard functions computing absolute values.
>>
>> If it's C-specific I'd expect it to be in c.opt (in the appropriate
>> alphabetical position) and have "C ObjC" instead of Common.
>
> I see, fixed.
>
>>
>>> +@item -Wabsolute-value
>>> +@opindex Wabsolute-value
>>> +@opindex Wno-absolute-value
>>> +Warn when a wrong absolute value function seems to be used or when it
>>> +does not have any effect because its argument is an unsigned type.
>>> +This warning is specific to C language and can be suppressed with an
>>> +explicit type cast. This warning is also enabled by @option{-Wextra}.
>>
>> And then the @item would have @r{(C and Objective-C only)}, similar to
>> other such options (rather than saying "specific to C language" in the
>> text).
>>
>
> Likewise.
>
> I have bootstrapped and tested the updated (and re-based) patch on
> x86-64-linux. OK for trunk now?
>
> Thank you very much,
>
> Martin
>
>
> 2018-09-03 Martin Jambor
>
> gcc/
> * doc/invoke.texi (Warning Options): Likewise.
>
> gcc/c-family/
> * c.opt (Wabsolute-value): New.
>
> gcc/c/
> * c-parser.c: (warn_for_abs): New function.
> (c_parser_postfix_expression_after_primary): Call it.
>
> testsuite/
> * gcc.dg/warn-abs-1.c: New test.
> * gcc.dg/dfp/warn-abs-2.c: Likewise.
OK.
jeff
Re: [PATCH] Add -Wabsolute-value
Hi,
On Fri, Aug 31 2018, Joseph Myers wrote:
> On Fri, 31 Aug 2018, Martin Jambor wrote:
>
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index ebc3ef43ce2..2950760fb2a 100644
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -815,6 +815,10 @@ Wvector-operation-performance
>> Common Var(warn_vector_operation_performance) Warning
>> Warn when a vector operation is compiled outside the SIMD.
>>
>> +Wabsolute-value
>> +Common Var(warn_absolute_value) Warning EnabledBy(Wextra)
>> +Warn on suspicious calls of standard functions computing absolute values.
>
> If it's C-specific I'd expect it to be in c.opt (in the appropriate
> alphabetical position) and have "C ObjC" instead of Common.
I see, fixed.
>
>> +@item -Wabsolute-value
>> +@opindex Wabsolute-value
>> +@opindex Wno-absolute-value
>> +Warn when a wrong absolute value function seems to be used or when it
>> +does not have any effect because its argument is an unsigned type.
>> +This warning is specific to C language and can be suppressed with an
>> +explicit type cast. This warning is also enabled by @option{-Wextra}.
>
> And then the @item would have @r{(C and Objective-C only)}, similar to
> other such options (rather than saying "specific to C language" in the
> text).
>
Likewise.
I have bootstrapped and tested the updated (and re-based) patch on
x86-64-linux. OK for trunk now?
Thank you very much,
Martin
2018-09-03 Martin Jambor
gcc/
* doc/invoke.texi (Warning Options): Likewise.
gcc/c-family/
* c.opt (Wabsolute-value): New.
gcc/c/
* c-parser.c: (warn_for_abs): New function.
(c_parser_postfix_expression_after_primary): Call it.
testsuite/
* gcc.dg/warn-abs-1.c: New test.
* gcc.dg/dfp/warn-abs-2.c: Likewise.
---
gcc/c-family/c.opt| 4 +
gcc/c/c-parser.c | 156 +-
gcc/doc/invoke.texi | 8 ++
gcc/testsuite/gcc.dg/dfp/warn-abs-2.c | 28 +
gcc/testsuite/gcc.dg/warn-abs-1.c | 67 +++
5 files changed, 257 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
create mode 100644 gcc/testsuite/gcc.dg/warn-abs-1.c
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 31a2b972919..092ec940d86 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -271,6 +271,10 @@ Warn if a subobject has an abi_tag attribute that the
complete object type does
Wpsabi
C ObjC C++ ObjC++ LTO Var(warn_psabi) Init(1) Warning Undocumented
LangEnabledBy(C ObjC C++ ObjC++,Wabi)
+Wabsolute-value
+C ObjC Var(warn_absolute_value) Warning EnabledBy(Wextra)
+Warn on suspicious calls of standard functions computing absolute values.
+
Waddress
C ObjC C++ ObjC++ Var(warn_address) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
Warn about suspicious uses of memory addresses.
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 69ed5ae9d2f..1766a256633 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -9101,6 +9101,144 @@ sizeof_ptr_memacc_comptypes (tree type1, tree type2)
return comptypes (type1, type2) == 1;
}
+/* Warn for patterns where abs-like function appears to be used incorrectly,
+ gracely ignore any non-abs-like function. The warning location should be
+ LOC. FNDECL is the declaration of called function, it must be a
+ BUILT_IN_NORMAL function. ARG is the first and only argument of the
+ call. */
+
+static void
+warn_for_abs (location_t loc, tree fndecl, tree arg)
+{
+ tree atype = TREE_TYPE (arg);
+
+ /* Casts from pointers (and thus arrays and fndecls) will generate
+ -Wint-conversion warnings. Most other wrong types hopefully lead to type
+ mismatch errors. TODO: Think about what to do with FIXED_POINT_TYPE_P
+ types and possibly other exotic types. */
+ if (!INTEGRAL_TYPE_P (atype)
+ && !SCALAR_FLOAT_TYPE_P (atype)
+ && TREE_CODE (atype) != COMPLEX_TYPE)
+return;
+
+ enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
+
+ switch (fcode)
+{
+case BUILT_IN_ABS:
+case BUILT_IN_LABS:
+case BUILT_IN_LLABS:
+case BUILT_IN_IMAXABS:
+ if (!INTEGRAL_TYPE_P (atype))
+ {
+ if (SCALAR_FLOAT_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using integer absolute value function %qD when "
+ "argument is of floating point type %qT",
+ fndecl, atype);
+ else if (TREE_CODE (atype) == COMPLEX_TYPE)
+ warning_at (loc, OPT_Wabsolute_value,
+ "using integer absolute value function %qD when "
+ "argument is of complex type %qT", fndecl, atype);
+ else
+ gcc_unreachable ();
+ return;
+ }
+ if (TYPE_UNSIGNED (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "taking the absolute value of unsigned type %qT "
+
Re: [PATCH] Add -Wabsolute-value
On Fri, 31 Aug 2018, Martin Jambor wrote:
> diff --git a/gcc/common.opt b/gcc/common.opt
> index ebc3ef43ce2..2950760fb2a 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -815,6 +815,10 @@ Wvector-operation-performance
> Common Var(warn_vector_operation_performance) Warning
> Warn when a vector operation is compiled outside the SIMD.
>
> +Wabsolute-value
> +Common Var(warn_absolute_value) Warning EnabledBy(Wextra)
> +Warn on suspicious calls of standard functions computing absolute values.
If it's C-specific I'd expect it to be in c.opt (in the appropriate
alphabetical position) and have "C ObjC" instead of Common.
> +@item -Wabsolute-value
> +@opindex Wabsolute-value
> +@opindex Wno-absolute-value
> +Warn when a wrong absolute value function seems to be used or when it
> +does not have any effect because its argument is an unsigned type.
> +This warning is specific to C language and can be suppressed with an
> +explicit type cast. This warning is also enabled by @option{-Wextra}.
And then the @item would have @r{(C and Objective-C only)}, similar to
other such options (rather than saying "specific to C language" in the
text).
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH] Add -Wabsolute-value
Hi,
thank you very much for your comments.
On Fri, Aug 24 2018, Joseph Myers wrote:
> On Fri, 24 Aug 2018, Martin Jambor wrote:
>
>> +/* Assuming we have encountered a call to a probably wrong kind of abs,
>> issue a
>> + warning. LOC is the location of the call, FNKIND is a string
>> characterizing
>> + the class of the used abs function, FNDEC is the actual function
>> declaration
>> + and ATYPE is type of the supplied actual argument. */
>
> For proper i18n, you have to use an enumeration here, not English string
> fragments.
>
>> + warning_at (loc, OPT_Wabsolute_value,
>> + "using %s absolute value function %qD when argument "
>> + "is of %s type %qT", fnkind, fndecl, act, atype);
>
> And then have all the possible combinations as complete sentences, in
> separate warning_at calls in appropriate switch statments or marked up
> with G_() if you put more than one in a single warning_at call using ?:,
> for translation purposes. (Any cases that are impossible combinations for
> the warning - you don't want translators to have to produce useless
> translations where e.g. both argument and call are of the same kind and so
> the warning shouldn't occur - should use gcc_unreachable ().)
Understood. That however defeats the purpose of having the warning
string generation in a separate function and so I removed it and just
put separate warning_at calls with the entire string in warn_for_abs.
>
>> +case BUILT_IN_FABS:
>> +case BUILT_IN_FABSF:
>> +case BUILT_IN_FABSL:
>> + if (!SCALAR_FLOAT_TYPE_P (atype)
>> + || DECIMAL_FLOAT_MODE_P (TYPE_MODE (atype)))
>
> Should include the _FloatN / _FloatNx built-in functions here (CASE_FLT_FN
> together with CASE_FLT_FN_FLOATN_NX).
OK, fixed.
>
>> +case BUILT_IN_CABS:
>> +case BUILT_IN_CABSF:
>> +case BUILT_IN_CABSL:
>
> And use CASE_FLT_FN here rather than hardcoding the list (but we don't yet
> have _FloatN / _FloatNx built-in variants of cabs).
Likewise.
>
>> + tree ftype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
>> + if (tree_fits_uhwi_p (TYPE_SIZE (atype))
>> + && tree_to_uhwi (TYPE_SIZE (atype)) > tree_to_uhwi (TYPE_SIZE
>> (ftype)))
>> +warning_at (loc, OPT_Wabsolute_value,
>> +"absolute value function %qD given an argument of type %qT "
>> +"but has parameter of type %qT which may cause truncation "
>> +"of value ", fndecl, atype, ftype);
>
> Should not have space at end of warning text. I don't think TYPE_SIZE is
> the right thing to use in general; for example, on x86_64, you should warn
> for passing a _Float128 value to fabsl, but both long double and _Float128
> are 16-byte types (with only 10 bytes non-padding in long double).
I have looked at how unsafe_conversion_p detects these cases and it
seems it relies entirely on TYPE_PRECISION, so I changed my code to do
the same. I have also added a test for long double vs _Float128 to the
first test case.
Bootstrapped and tested on x86_64-linux. What do you think about it
now?
Thanks,
Martin
2018-08-29 Martin Jambor
gcc/
* common.opt (Wabsolute-value): New.
* doc/invoke.texi (Warning Options): Likewise.
gcc/c/
(warn_for_abs): New function.
(c_parser_postfix_expression_after_primary): Call it.
testsuite/
* gcc.dg/warn-abs-1.c: New test.
* gcc.dg/dfp/warn-abs-2.c: Likewise.
---
gcc/c/c-parser.c | 155 +-
gcc/common.opt| 4 +
gcc/doc/invoke.texi | 8 ++
gcc/testsuite/gcc.dg/dfp/warn-abs-2.c | 28 +
gcc/testsuite/gcc.dg/warn-abs-1.c | 67 +++
5 files changed, 256 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
create mode 100644 gcc/testsuite/gcc.dg/warn-abs-1.c
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 0d5dbea8f67..c180d9a391e 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -9110,6 +9110,144 @@ sizeof_ptr_memacc_comptypes (tree type1, tree type2)
return comptypes (type1, type2) == 1;
}
+/* Warn for patterns where abs-like function appears to be used incorrectly,
+ gracely ignore any non-abs-like function. The warning location should be
+ LOC. FNDECL is the declaration of called function, it must be a
+ BUILT_IN_NORMAL function. ARG is the first and only argument of the
+ call. */
+
+static void
+warn_for_abs (location_t loc, tree fndecl, tree arg)
+{
+ tree atype = TREE_TYPE (arg);
+
+ /* Casts from pointers (and thus arrays and fndecls) will generate
+ -Wint-conversion warnings. Most other wrong types hopefully lead to type
+ mismatch errors. TODO: Think about what to do with FIXED_POINT_TYPE_P
+ types and possibly other exotic types. */
+ if (!INTEGRAL_TYPE_P (atype)
+ && !SCALAR_FLOAT_TYPE_P (atype)
+ && TREE_CODE (atype) != COMPLEX_TYPE)
+return;
+
+ enum
Re: [PATCH] Add -Wabsolute-value
On Fri, 24 Aug 2018, Martin Jambor wrote: > +/* Assuming we have encountered a call to a probably wrong kind of abs, > issue a > + warning. LOC is the location of the call, FNKIND is a string > characterizing > + the class of the used abs function, FNDEC is the actual function > declaration > + and ATYPE is type of the supplied actual argument. */ For proper i18n, you have to use an enumeration here, not English string fragments. > + warning_at (loc, OPT_Wabsolute_value, > + "using %s absolute value function %qD when argument " > + "is of %s type %qT", fnkind, fndecl, act, atype); And then have all the possible combinations as complete sentences, in separate warning_at calls in appropriate switch statments or marked up with G_() if you put more than one in a single warning_at call using ?:, for translation purposes. (Any cases that are impossible combinations for the warning - you don't want translators to have to produce useless translations where e.g. both argument and call are of the same kind and so the warning shouldn't occur - should use gcc_unreachable ().) > +case BUILT_IN_FABS: > +case BUILT_IN_FABSF: > +case BUILT_IN_FABSL: > + if (!SCALAR_FLOAT_TYPE_P (atype) > + || DECIMAL_FLOAT_MODE_P (TYPE_MODE (atype))) Should include the _FloatN / _FloatNx built-in functions here (CASE_FLT_FN together with CASE_FLT_FN_FLOATN_NX). > +case BUILT_IN_CABS: > +case BUILT_IN_CABSF: > +case BUILT_IN_CABSL: And use CASE_FLT_FN here rather than hardcoding the list (but we don't yet have _FloatN / _FloatNx built-in variants of cabs). > + tree ftype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl))); > + if (tree_fits_uhwi_p (TYPE_SIZE (atype)) > + && tree_to_uhwi (TYPE_SIZE (atype)) > tree_to_uhwi (TYPE_SIZE (ftype))) > +warning_at (loc, OPT_Wabsolute_value, > + "absolute value function %qD given an argument of type %qT " > + "but has parameter of type %qT which may cause truncation " > + "of value ", fndecl, atype, ftype); Should not have space at end of warning text. I don't think TYPE_SIZE is the right thing to use in general; for example, on x86_64, you should warn for passing a _Float128 value to fabsl, but both long double and _Float128 are 16-byte types (with only 10 bytes non-padding in long double). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] Add -Wabsolute-value
Hi
On Wed, Aug 15 2018, Eric Gallager wrote:
> On 8/14/18, Joseph Myers wrote:
>> On Tue, 14 Aug 2018, Martin Jambor wrote:
>>
>>> when you try compiling a call to function abs and provide an unsigned
>>> int in the argument in C++, you will get an error about ambiguous
>>> overload. In C however, it will pass without silently. The following
>>> patch adds a warning for these cases, because I think it is likely that
>>> such code does not do what the author intended.
>>
>> abs of unsigned short (promoted to int) seems harmless; I don't see any
>> tests to make sure it doesn't warn. Really the issue seems more like abs
>> (or labs / llabs / imaxabs) of an argument whose value might be changed by
>> the conversion to int.
That actually wasn't my motivation. I believe that when someone
computes absolute value of something, they expect that the something can
have negative values, which however is not true if its type is unsigned
and so they might want to go and re-check the code and/or data
structures. Therefore, I'd prefer to warn in all such cases.
>>> + else if (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_ABS
>>
>> This looks like it would only handle abs, not labs / llabs / imaxabs.
Right, I originally only planned to send the patch only as an RFC and
forgot to add them when I made it more complete. Sorry.
>>
>>> +@code{<} or @code{>=}. When compiling C, also warn when calculating
>>> +an absolute value from an unsigned type. This warning is also enabled
>>
>> But this would suggest any absolute value function, not just abs.
>
> clang has a -Wabsolute-value warning flag that might be looking at
> here for comparison; see bug 63886 for an example:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63886
Interesting. If that's the case, maybe we can have an extra option too.
The following patch adds it, with most of Clang's functionality, the
missing bits (effectively passing a pointer to abs) are already errors
or dealt with -Wint-conversion which is in -Wall. It:
1) warns if an unsigned integer type is passed,
2) divides abs functions into integer, floating-point, complex and
decimal-floating-point classes and warns if a wrong one is selected,
and
3) warns if an absolute value function is passed a bigger actual
argument than its formal parameter, e.g. if abs() is used where
labs() seems the appropriate choice
The 2) and 3) functionality overlaps quite a bit with
-Wfloat-conversion, but they also warn on situations not covered there
(abs() instead of labs() is probably the best example). I can remove
those bits if it was deemed a problem, but I thought that symmetrical
behavior is better, especially because -Wfloat-conversion is not even in
-Wextra.
The warning is part of -Wextra. Like before, all warnings can be
suppressed with an explicit type cast. Therefore I still warn early,
not attempting any use of VRP as suggested by Martin Sebor.
What do you think, is this potentially useful? Should I remove some
bits? Any other suggestion/comments? For the record, the patch passes
bootstrap and testing on x86_64-linux.
Thanks,
Martin
2018-08-23 Martin Jambor
gcc/
* common.opt (Wabsolute-value): New.
* doc/invoke.texi (Warning Options): Likewise.
gcc/c/
* c/c-parser.c (construct_wrong_abs_kind_warning): New function.
(warn_for_abs): Likewise.
(c_parser_postfix_expression_after_primary): Call it.
testsuite/
* gcc.dg/warn-abs-1.c: New test.
* gcc.dg/dfp/warn-abs-2.c: Likewise.
---
gcc/c/c-parser.c | 132 --
gcc/common.opt| 4 +
gcc/doc/invoke.texi | 8 ++
gcc/testsuite/gcc.dg/dfp/warn-abs-2.c | 21
gcc/testsuite/gcc.dg/warn-abs-1.c | 66 +
5 files changed, 225 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
create mode 100644 gcc/testsuite/gcc.dg/warn-abs-1.c
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 5ad4f57a4fe..46adace69c5 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -9104,6 +9104,121 @@ sizeof_ptr_memacc_comptypes (tree type1, tree type2)
return comptypes (type1, type2) == 1;
}
+/* Assuming we have encountered a call to a probably wrong kind of abs, issue a
+ warning. LOC is the location of the call, FNKIND is a string characterizing
+ the class of the used abs function, FNDEC is the actual function declaration
+ and ATYPE is type of the supplied actual argument. */
+
+static void
+construct_wrong_abs_kind_warning (location_t loc, const char *fnkind,
+ tree fndecl, tree atype)
+{
+ const char *act;
+
+ if (INTEGRAL_TYPE_P (atype))
+act = "integer";
+ else if (SCALAR_FLOAT_TYPE_P (atype))
+{
+ if (DECIMAL_FLOAT_MODE_P (TYPE_MODE (atype)))
+ act = "decimal floating point";
+ else
+ act = "floating point";
+}
+ else if
