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 <mjam...@suse.cz>
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 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 "
+ "has no effect", atype);
+ break;
+
+ CASE_FLT_FN (BUILT_IN_FABS):
+ CASE_FLT_FN_FLOATN_NX (BUILT_IN_FABS):
+ if (!SCALAR_FLOAT_TYPE_P (atype)
+ || DECIMAL_FLOAT_MODE_P (TYPE_MODE (atype)))
+ {
+ if (INTEGRAL_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using floating point absolute value function %qD "
+ "when argument is of integer type %qT", fndecl, atype);
+ else if (DECIMAL_FLOAT_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using floating point absolute value function %qD "
+ "when argument is of decimal floating point type %qT",
+ fndecl, atype);
+ else if (TREE_CODE (atype) == COMPLEX_TYPE)
+ warning_at (loc, OPT_Wabsolute_value,
+ "using floating point absolute value function %qD when "
+ "argument is of complex type %qT", fndecl, atype);
+ else
+ gcc_unreachable ();
+ return;
+ }
+ break;
+
+ CASE_FLT_FN (BUILT_IN_CABS):
+ if (TREE_CODE (atype) != COMPLEX_TYPE)
+ {
+ if (INTEGRAL_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using complex absolute value function %qD when "
+ "argument is of integer type %qT", fndecl, atype);
+ else if (SCALAR_FLOAT_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using complex absolute value function %qD when "
+ "argument is of floating point type %qT",
+ fndecl, atype);
+ else
+ gcc_unreachable ();
+
+ return;
+ }
+ break;
+
+ case BUILT_IN_FABSD32:
+ case BUILT_IN_FABSD64:
+ case BUILT_IN_FABSD128:
+ if (!DECIMAL_FLOAT_TYPE_P (atype))
+ {
+ if (INTEGRAL_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using decimal floating point absolute value "
+ "function %qD when argument is of integer type %qT",
+ fndecl, atype);
+ else if (SCALAR_FLOAT_TYPE_P (atype))
+ warning_at (loc, OPT_Wabsolute_value,
+ "using decimal floating point 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 decimal floating point absolute value "
+ "function %qD when argument is of complex type %qT",
+ fndecl, atype);
+ else
+ gcc_unreachable ();
+ return;
+ }
+ break;
+
+ default:
+ return;
+ }
+
+ tree ftype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
+ if (TREE_CODE (atype) == COMPLEX_TYPE)
+ {
+ gcc_assert (TREE_CODE (ftype) == COMPLEX_TYPE);
+ atype = TREE_TYPE (atype);
+ ftype = TREE_TYPE (ftype);
+ }
+
+ if (TYPE_PRECISION (ftype) < TYPE_PRECISION (atype))
+ 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.
@@ -9175,13 +9313,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 985ef9f3510..5720841168c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -6281,6 +6281,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..c1a1994997f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/dfp/warn-abs-2.c
@@ -0,0 +1,28 @@
+/* { 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" } */
+}
+
+void
+test_size (_Decimal64 *p64, _Decimal128 *p128)
+{
+ *p64 = __builtin_fabsd32 (*p64); /* { dg-warning "may cause truncation of
value" } */
+ *p128 = __builtin_fabsd64 (*p128); /* { dg-warning "may cause truncation of
value" } */
+}
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..6aa937c3a2e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/warn-abs-1.c
@@ -0,0 +1,67 @@
+/* { 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, _Float128 *pf128)
+{
+ *pd = fabsf (*pd); /* { dg-warning "may cause truncation of value" } */
+ *pld = fabs (*pld); /* { dg-warning "may cause truncation of value" } */
+ *pld = fabs ((double) *pld);
+ *pf128 = fabsl (*pf128); /* { dg-warning "may cause truncation of value" } */
+}
+
+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
