| Message ID | ri6k1otupt9.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [C] Warn when calculating abs(unsigned_value) | expand |
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. Except that's more like a subset of -Wconversion, and highly likely to have false positives (for a long argument that is actually always in the range of int), so maybe a restriction to unsigned arguments makes sense for this warning, but should still only be for unsigned arguments at least as wide as the argument type of the abs function in question. > + else if (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_ABS This looks like it would only handle abs, not labs / llabs / imaxabs. > +@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.
On 8/14/18, Joseph Myers <joseph@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. Except that's more like a subset of -Wconversion, > and highly likely to have false positives (for a long argument that is > actually always in the range of int), so maybe a restriction to unsigned > arguments makes sense for this warning, but should still only be for > unsigned arguments at least as wide as the argument type of the abs > function in question. > >> + else if (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_ABS > > This looks like it would only handle abs, not labs / llabs / imaxabs. > >> +@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 > > -- > Joseph S. Myers > joseph@codesourcery.com >
On 08/14/2018 05:53 AM, Martin Jambor wrote: > Hi, > > 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. > > I thought it a bit of an overkill to add a new warning group because of > this, so I added it to -Wtype-limits, which seemed the best fit. As a > consequence, the warning is on with -Wextra. The warning can be > suppressed with an explicit type-cast (which at the moment is implicit), > if someone really used it for some bit-tricks. > > Bootstrapped and tested on x86_64-linux, also with make info. What do > you think, is this a good idea? Is it perhaps OK for trunk? [Sorry, this turned into a longer response than I had planned (and than I suspect you expected.] The C++ errors are the result of the mess C++ has made of things because of all the overloads it adds in different headers (I don't say this to bash the language -- far from it). So I wouldn't look to those as a model for warnings. That said, I agree that calling abs() with an unsigned argument could be a bug. Even calling abs() with a signed argument may not be safe: abs(INT_MIN) is undefined, and passing in a wider type will slice off the high end bits. Detecting some of these bugs without too much noise would require moving the warning out of the front-end and to some later point after VRP has run. > + else if (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_ABS > + && vec_safe_length (exprlist) == 1 > + && TYPE_UNSIGNED (TREE_TYPE ((*exprlist)[0]))) > + warning_at (expr_loc, OPT_Wtype_limits, > + "calculation of an absolute value of " > + "a value of unsigned type"); I would suggest to include more detail about the argument: at a minimum, its type, and in the constant case (or in the middle-end, when the range of the argument is known), its value/range. Following the example in overflow_warning() the warning in the constant case might look something along the lines of: warning_at (expr_loc, "conversion in an %qD expression " "with argument %E of type %T" results in %E", expr.value, arg0, TREE_TYPE (arg0), result); and something like the following for arguments in a known range (with VRP): warning_at (expr_loc, "conversion in an %qD expression " "with argument between %E and %E of type %T" " "results in a negative value" between %E and %E, expr.value, arg0_min, arg0_max, TREE_TYPE (arg0), result_min, result_max); (As an aside, the term "calculate" is more commonly used colloquially and less frequently in reference to computers or mathematics than "compute." In gcc.pot, there are just 6 occurrences of the former and 21 of the latter.) Martin
On Wed, 15 Aug 2018, Martin Sebor wrote: > Detecting some of these bugs without too much noise would require > moving the warning out of the front-end and to some later point > after VRP has run. But you need the information about whether the conversion was explicit or implicit. If someone does abs ((int) unsigned_var) it's presumably intentional and should not receive a warning. If they do abs (unsigned_var) it's suspect. If they do abs (long_var) it's entirely possible they know the long value is within range for int, but it came from some API that produces long.
On 08/16/2018 10:49 AM, Joseph Myers wrote: > On Wed, 15 Aug 2018, Martin Sebor wrote: > >> Detecting some of these bugs without too much noise would require >> moving the warning out of the front-end and to some later point >> after VRP has run. > > But you need the information about whether the conversion was explicit or > implicit. If someone does > > abs ((int) unsigned_var) > > it's presumably intentional and should not receive a warning. If they do > > abs (unsigned_var) > > it's suspect. If they do > > abs (long_var) > > it's entirely possible they know the long value is within range for int, > but it came from some API that produces long. Right. The warning would need to avoid triggering for arguments in an unknown range, or in a range that straddles the boundary between positive and negative values. But the lack of a distinction between implicit and explicit casts int the middle-end is a problem, not just here but in other contexts. Maybe NOP_EXPR could be extended to capture it. There should be an unused bit there somewhere. I suspect this would be far more work than could be justified for a single warning, but it may be an idea for a more general design enhancement. Martin
Hi On Wed, Aug 15 2018, Eric Gallager wrote: > On 8/14/18, Joseph Myers <joseph@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 <mjambor@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); +} + + + +
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).
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 <mjambor@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); +} + + + +
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).
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 <mjambor@suse.cz> 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 " + "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. @@ -9165,13 +9303,19 @@ c_parser_postfix_expression_after_primary (c_parser *parser, expr.value, exprlist, sizeof_arg, sizeof_ptr_memacc_comptypes); - if (TREE_CODE (expr.value) == FUNCTION_DECL - && fndecl_built_in_p (expr.value, BUILT_IN_MEMSET) - && vec_safe_length (exprlist) == 3) + if (TREE_CODE (expr.value) == FUNCTION_DECL) { - tree arg0 = (*exprlist)[0]; - tree arg2 = (*exprlist)[2]; - warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask); + if (fndecl_built_in_p (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); + } + if (warn_absolute_value + && fndecl_built_in_p (expr.value, BUILT_IN_NORMAL) + && vec_safe_length (exprlist) == 1) + warn_for_abs (expr_loc, expr.value, (*exprlist)[0]); } start = expr.get_start (); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f911ae0d1af..953a26ed226 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 @r{(C and Objective-C only)} +@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 be suppressed with an explicit type cast and it 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); +} + + + +
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 <mjambor@suse.cz> > > 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
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 7a926285f3a..bf3b8cc75f9 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -9160,13 +9160,21 @@ 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 (DECL_FUNCTION_CODE (expr.value) == BUILT_IN_ABS + && vec_safe_length (exprlist) == 1 + && TYPE_UNSIGNED (TREE_TYPE ((*exprlist)[0]))) + warning_at (expr_loc, OPT_Wtype_limits, + "calculation of an absolute value of " + "a value of unsigned type"); } start = expr.get_start (); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index d7fd0e17555..b757ac0d0c8 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -6179,8 +6179,9 @@ This warning is enabled by default. Warn if a comparison is always true or always false due to the limited range of the data type, but do not warn for constant expressions. For example, warn if an unsigned variable is compared against zero with -@code{<} or @code{>=}. This warning is also enabled by -@option{-Wextra}. +@code{<} or @code{>=}. When compiling C, also warn when calculating +an absolute value from an unsigned type. This warning is also enabled +by @option{-Wextra}. @include cppwarnopts.texi diff --git a/gcc/testsuite/gcc.dg/warn-abs-unsigned-1.c b/gcc/testsuite/gcc.dg/warn-abs-unsigned-1.c new file mode 100644 index 00000000000..8d2a6d87e6b --- /dev/null +++ b/gcc/testsuite/gcc.dg/warn-abs-unsigned-1.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-Wtype-limits" } */ + +#include <stdlib.h> + +unsigned __attribute__((noipa)) +get_input (void) +{ + return 22; +} + +volatile unsigned g; + +int +main(int argc __attribute__((unused)), char **argv __attribute__((unused))) +{ + unsigned u = get_input (); + u = abs (u); /* { dg-warning "calculation of an absolute value of a value of unsigned type" } */ + g = u; + return 0; +}