This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [C] Warn when calculating abs(unsigned_value)

On 08/14/2018 05:53 AM, 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.

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.)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]