Hi
On Wed, Aug 15 2018, Eric Gallager wrote:
> On 8/14/18, Joseph Myers <jos...@codesourcery.com> 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 <mjam...@suse.cz>
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 (TREE_CODE (atype) == COMPLEX_TYPE)
+ act = " complex";
+ else
+ gcc_unreachable ();
+ warning_at (loc, OPT_Wabsolute_value,
+ "using %s absolute value function %qD when argument "
+ "is of %s type %qT", fnkind, fndecl, act, atype);
+}
+
+/* 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))
+ {
+ construct_wrong_abs_kind_warning (loc, "integer", fndecl, atype);
+ return;
+ }
+ if (TYPE_UNSIGNED (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "taking the absolute value of unsigned type %qT "
+ "has no effect", atype);
+ break;
+
+ 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)))
+ {
+ construct_wrong_abs_kind_warning (loc, "floating point", fndecl,
+ atype);
+ return;
+ }
+ break;
+
+ case BUILT_IN_CABS:
+ case BUILT_IN_CABSF:
+ case BUILT_IN_CABSL:
+ if (TREE_CODE (atype) != COMPLEX_TYPE)
+ {
+ construct_wrong_abs_kind_warning (loc, "complex", fndecl, atype);
+ return;
+ }
+ break;
+
+ case BUILT_IN_FABSD32:
+ case BUILT_IN_FABSD64:
+ case BUILT_IN_FABSD128:
+ if (!DECIMAL_FLOAT_TYPE_P (atype))
+ {
+ construct_wrong_abs_kind_warning (loc, "decimal floating point",
+ fndecl, atype);
+ return;
+ }
+ break;
+
+ default:
+ return;
+ }
+
+ 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);
+}
+
+
/* Parse a postfix expression after the initial primary or compound
literal; that is, parse a series of postfix operators.
@@ -9169,13 +9284,18 @@ c_parser_postfix_expression_after_primary (c_parser
*parser,
sizeof_arg,
sizeof_ptr_memacc_comptypes);
if (TREE_CODE (expr.value) == FUNCTION_DECL
- && DECL_BUILT_IN_CLASS (expr.value) == BUILT_IN_NORMAL
- && DECL_FUNCTION_CODE (expr.value) == BUILT_IN_MEMSET
- && vec_safe_length (exprlist) == 3)
+ && DECL_BUILT_IN_CLASS (expr.value) == BUILT_IN_NORMAL)
{
- tree arg0 = (*exprlist)[0];
- tree arg2 = (*exprlist)[2];
- warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask);
+ if (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_MEMSET
+ && vec_safe_length (exprlist) == 3)
+ {
+ tree arg0 = (*exprlist)[0];
+ tree arg2 = (*exprlist)[2];
+ warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask);
+ }
+ else if (warn_absolute_value
+ && vec_safe_length (exprlist) == 1)
+ warn_for_abs (expr_loc, expr.value, (*exprlist)[0]);
}
start = expr.get_start ();
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.
+
Xassembler
Driver Separate
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index f8287153be1..d6babfcafa0 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -6209,6 +6209,14 @@ example, warn if an unsigned variable is compared
against zero with
@code{<} or @code{>=}. This warning is also enabled by
@option{-Wextra}.
+@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}.
+
@include cppwarnopts.texi
@item -Wbad-function-cast @r{(C and Objective-C only)}
diff --git a/gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
b/gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
new file mode 100644
index 00000000000..40778907ab5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-options "-Wabsolute-value" } */
+
+#include <stdlib.h>
+#include <complex.h>
+#include <math.h>
+
+void tst_decimal (_Decimal32 *p32, _Decimal64 *p64, _Decimal128 *p128)
+{
+ *p32 = abs(*p32); /* { dg-warning "using integer absolute value
function" } */
+ *p64 = fabs(*p64); /* { dg-warning "using floating point absolute value
function" } */
+ *p128 = cabsl(*p128); /* { dg-warning "using complex absolute value
function" } */
+}
+
+void tst_notdecimal (int *pi, double *pd, long double *pld, complex double *pc)
+{
+ *pi = __builtin_fabsd32 (*pi); /* { dg-warning "using decimal floating
point absolute value function" } */
+ *pd = __builtin_fabsd64 (*pd); /* { dg-warning "using decimal floating
point absolute value function" } */
+ *pld = __builtin_fabsd64 (*pld); /* { dg-warning "using decimal floating
point absolute value function" } */
+ *pc = __builtin_fabsd128 (*pc); /* { dg-warning "using decimal floating
point absolute value function" } */
+}
diff --git a/gcc/testsuite/gcc.dg/warn-abs-1.c
b/gcc/testsuite/gcc.dg/warn-abs-1.c
new file mode 100644
index 00000000000..1c487270042
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/warn-abs-1.c
@@ -0,0 +1,66 @@
+/* { dg-do compile } */
+/* { dg-options "-Wabsolute-value" } */
+
+#include <stdlib.h>
+#include <inttypes.h>
+#include <math.h>
+#include <complex.h>
+
+void
+tst_unsigned (unsigned *pu, unsigned long *pl, unsigned long long *pll,
+ uintmax_t *pm)
+{
+ *pu = abs (*pu); /* { dg-warning "taking the absolute value of unsigned
type" } */
+ *pl = labs (*pl); /* { dg-warning "taking the absolute value of unsigned
type" } */
+ *pll = llabs (*pll); /* { dg-warning "taking the absolute value of unsigned
type" } */
+ *pm = imaxabs (*pm); /* { dg-warning "taking the absolute value of
unsigned type" } */
+}
+
+void
+test_int_size (long long *pll)
+{
+ *pll = abs (*pll); /* { dg-warning "may cause truncation of value" } */
+ *pll = abs ((int) *pll);
+}
+
+void
+tst_notint (float *pf, double *pd, _Complex double *pc)
+{
+ *pf = abs (*pf); /* { dg-warning "using integer absolute value function"
} */
+ *pd = labs (*pd); /* { dg-warning "using integer absolute value function"
} */
+ *pc = abs (*pc); /* { dg-warning "using integer absolute value function"
} */
+}
+
+void
+tst_notfloat (int *pi, long *pl, complex double *pc)
+{
+ *pi = fabsf (*pi); /* { dg-warning "using floating point absolute value
function" } */
+ *pl = fabs (*pl); /* { dg-warning "using floating point absolute value
function" } */
+ *pc = fabs (*pc); /* { dg-warning "using floating point absolute value
function" } */
+}
+
+void
+tst_float_size (double *pd, long double *pld)
+{
+ *pd = fabsf (*pd); /* { dg-warning "may cause truncation of value" } */
+ *pld = fabs (*pld); /* { dg-warning "may cause truncation of value" } */
+ *pld = fabs ((double) *pld);
+}
+
+void tst_notcomplex (int *pi, long *pl, long double *pld)
+{
+ *pi = cabs (*pi); /* { dg-warning "using complex absolute value function"
} */
+ *pl = cabs (*pl); /* { dg-warning "using complex absolute value function"
} */
+ *pld = cabsl (*pld);/* { dg-warning "using complex absolute value function"
} */
+}
+
+void tst_cplx_size (complex double *pcd, complex long double *pcld)
+{
+ *pcd = cabsf (*pcd); /* { dg-warning "may cause truncation of value" } */
+ *pcld = cabs (*pcld); /* { dg-warning "may cause truncation of value" } */
+ *pcld = cabs ((complex double) *pcld);
+}
+
+
+
+
--
2.18.0
