Re: [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181]
On Mon, 2022-08-15 at 14:35 +0200, Tim Lange wrote:
> This patch fixes the ICE reported in PR106181 and adds a new warning
> to
> the analyzer complaining about the use of floating point operands.
Thanks for the patch.
Various comments inline...
>
> I decided to move the warning for floats inside the size argument out
> of
> the allocation size checker code and toward the allocation such that
> the
> warning only appears once.
> I'm not sure about the wording of the diagnostic, my current wording
> feels
> a bit bulky.
Agreed, and the warning itself is probably setting a new record for
option length: -Wanalyzer-imprecise-floating-point-arithmetic is 45
characters. I'm not without sin here: I think the current record is -
Wanalyzer-unsafe-call-within-signal-handler, which is 43.
How about:
-Wanalyzer-imprecise-float-arithmetic
-Wanalyzer-imprecise-fp-arithmetic
instead? (ideas welcome)
> Here is how the diagnostics look like:
>
> /path/to/main.c: In function ‘test_1’:
> /path/to/main.c:10:14: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results
https://gcc.gnu.org/codingconventions.html#Spelling
says we should use "floating-point" rather than "floating point".
How about just this:
"warning: use of floating-point arithmetic here might yield unexpected
results"
here...
> [-Wanalyzer-imprecise-floating-point-arithmetic]
> 10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 }
> */
> | ^
> ‘test_1’: event 1
> |
> | 10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line
> test_1 } */
> | | ^
> | | |
> | | (1) operand ‘n’ is of type ‘float’
> |
> /path/to/main.c:10:14: note: only use operands of a type that
> represents whole numbers inside the size argument
...and make the note say:
"only use operands of an integer type inside the size argument"
which tells the user that it's a size that we're complaining about.
> /path/to/main.c: In function ‘test_2’:
> /path/to/main.c:20:14: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results [-Wanalyzer-
> imprecise-floating-point-arithmetic]
> 20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
> | ^~~~
> ‘test_2’: event 1
> |
> | 20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
> | | ^~~~
> | | |
> | | (1) operand ‘3.1001e+0’ is of
> type ‘double’
> |
> /path/to/main.c:20:14: note: only use operands of a type that
> represents whole numbers inside the size argument
>
> Also, another point to discuss is the event note in case the
> expression is
> wrapped in a variable, such as in test_3:
> /path/to/main.c:30:10: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results [-Wanalyzer-
> imprecise-floating-point-arithmetic]
> 30 | return malloc (size); /* { dg-line test_3 } */
> | ^
> ‘test_3’: events 1-2
> |
> | 37 | void test_3 (float f)
> | | ^~
> | | |
> | | (1) entry to ‘test_3’
> | 38 | {
> | 39 | void *ptr = alloc_me (f); /* { dg-message "calling
> 'alloc_me' from 'test_3'" } */
> | |
> | | |
> | | (2) calling ‘alloc_me’ from ‘test_3’
> |
> +--> ‘alloc_me’: events 3-4
> |
> | 28 | void *alloc_me (size_t size)
> | | ^~~~
> | | |
> | | (3) entry to ‘alloc_me’
> | 29 | {
> | 30 | return malloc (size); /* { dg-line test_3 } */
> | | ~
> | | |
> | | (4) operand ‘f’ is of type ‘float’
> |
>
> I'm not sure if it is easily discoverable that event (4) does refer
> to
> 'size'. I thought about also printing get_representative_tree
> (capacity)
> in the event but that would clutter the event message if the argument
> does hold the full expression. I don't have any strong feelings about
> the
> decision here but if I had to decide I'd leave it as is (especially
> because the warning is probably quite unusual).
Yeah; get_representative_tree tries gets a tree, but trees don't give
us a good way of referring to a local var within a particular stack
frame within a path. So leaving it as is is OK.
> The index of the argument would also be a possibility, but that would
> get
> tricky for calloc.
[...snip...]
>
> diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
> index 61b58c575ff..bef15eae2c3 100644
> --- a/gcc/analyzer/analyzer.opt
> +++ b/gcc/analyzer/analyzer.opt
> @@ -98,6
[PATCH] analyzer: warn on the use of floating points in the size argument [PR106181]
This patch fixes the ICE reported in PR106181 and adds a new warning to
the analyzer complaining about the use of floating point operands.
I decided to move the warning for floats inside the size argument out of
the allocation size checker code and toward the allocation such that the
warning only appears once.
I'm not sure about the wording of the diagnostic, my current wording feels
a bit bulky. Here is how the diagnostics look like:
/path/to/main.c: In function ‘test_1’:
/path/to/main.c:10:14: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 } */
| ^
‘test_1’: event 1
|
| 10 | int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 } */
| | ^
| | |
| | (1) operand ‘n’ is of type ‘float’
|
/path/to/main.c:10:14: note: only use operands of a type that represents whole
numbers inside the size argument
/path/to/main.c: In function ‘test_2’:
/path/to/main.c:20:14: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
| ^~~~
‘test_2’: event 1
|
| 20 | int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
| | ^~~~
| | |
| | (1) operand ‘3.1001e+0’ is of type
‘double’
|
/path/to/main.c:20:14: note: only use operands of a type that represents whole
numbers inside the size argument
Also, another point to discuss is the event note in case the expression is
wrapped in a variable, such as in test_3:
/path/to/main.c:30:10: warning: use of floating point arithmetic inside the
size argument might yield unexpected results
[-Wanalyzer-imprecise-floating-point-arithmetic]
30 | return malloc (size); /* { dg-line test_3 } */
| ^
‘test_3’: events 1-2
|
| 37 | void test_3 (float f)
| | ^~
| | |
| | (1) entry to ‘test_3’
| 38 | {
| 39 | void *ptr = alloc_me (f); /* { dg-message "calling 'alloc_me'
from 'test_3'" } */
| |
| | |
| | (2) calling ‘alloc_me’ from ‘test_3’
|
+--> ‘alloc_me’: events 3-4
|
| 28 | void *alloc_me (size_t size)
| | ^~~~
| | |
| | (3) entry to ‘alloc_me’
| 29 | {
| 30 | return malloc (size); /* { dg-line test_3 } */
| | ~
| | |
| | (4) operand ‘f’ is of type ‘float’
|
I'm not sure if it is easily discoverable that event (4) does refer to
'size'. I thought about also printing get_representative_tree (capacity)
in the event but that would clutter the event message if the argument
does hold the full expression. I don't have any strong feelings about the
decision here but if I had to decide I'd leave it as is (especially
because the warning is probably quite unusual).
The index of the argument would also be a possibility, but that would get
tricky for calloc.
Regrtested on Linux x86_64, ran the analyzer & analyzer-torture tests with
the -m32 option enabled and had no false positives on coreutils, httpd,
openssh and curl.
2022-08-15 Tim Lange
gcc/analyzer/ChangeLog:
PR analyzer/106181
* analyzer.opt: Add Wanalyzer-imprecise-floating-point-arithmetic.
* region-model-impl-calls.cc (region_model::impl_call_alloca):
Add call to region_model::check_region_capacity_for_floats.
(region_model::impl_call_calloc):
Add call to region_model::check_region_capacity_for_floats.
(region_model::impl_call_malloc):
Add call to region_model::check_region_capacity_for_floats.
* region-model.cc (is_any_cast_p): Formatting.
(region_model::check_region_size): Ensure precondition.
(class imprecise_floating_point_arithmetic): New abstract
diagnostic class for all floating point related warnings.
(class float_as_size_arg): Concrete diagnostic class to complain
about floating point operands inside the size argument.
(class contains_floating_point_visitor):
New visitor to find floating point operands inside svalues.
(region_model::check_region_capacity_for_floats):
New function.
* region-model.h (class region_model):
Add region_model::check_region_capacity_for_floats.
gcc/ChangeLog:
PR analyzer/106181
* doc/invoke.texi:
Add Wan
