* [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181]
@ 2022-08-15 12:35 Tim Lange
2022-08-15 18:16 ` David Malcolm
2022-08-18 9:44 ` [PATCH v2] analyzer: warn on the use of floating-points operands " Tim Lange
0 siblings, 2 replies; 4+ messages in thread
From: Tim Lange @ 2022-08-15 12:35 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, Tim Lange
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.1000000000000001e+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 <mail@tim-lange.me>
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 Wanalyzer-imprecise-floating-point-arithmetic.
gcc/testsuite/ChangeLog:
PR analyzer/106181
* gcc.dg/analyzer/allocation-size-1.c: New test.
* gcc.dg/analyzer/imprecise-floating-point-1.c: New test.
* gcc.dg/analyzer/pr106181.c: New test.
---
gcc/analyzer/analyzer.opt | 4 +
gcc/analyzer/region-model-impl-calls.cc | 3 +
gcc/analyzer/region-model.cc | 192 ++++++++++++++++--
gcc/analyzer/region-model.h | 2 +
gcc/doc/invoke.texi | 14 ++
.../gcc.dg/analyzer/allocation-size-1.c | 10 +
.../analyzer/imprecise-floating-point-1.c | 62 ++++++
gcc/testsuite/gcc.dg/analyzer/pr106181.c | 11 +
8 files changed, 277 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
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 +98,10 @@ Wanalyzer-free-of-non-heap
Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning
Warn about code paths in which a non-heap pointer is freed.
+Wanalyzer-imprecise-floating-point-arithmetic
+Common Var(warn_analyzer_imprecise_floating_point_arithmetic) Init(1) Warning
+Warn about code paths in which floating point arithmetic is use in locations where precise computation is needed.
+
Wanalyzer-jump-through-null
Common Var(warn_analyzer_jump_through_null) Init(1) Warning
Warn about code paths in which a NULL function pointer is called.
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 8eebd122d42..c4d7e186963 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -237,6 +237,7 @@ region_model::impl_call_alloca (const call_details &cd)
const region *new_reg = create_region_for_alloca (size_sval, cd.get_ctxt ());
const svalue *ptr_sval
= m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+ check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
cd.maybe_set_lhs (ptr_sval);
}
@@ -393,6 +394,7 @@ region_model::impl_call_calloc (const call_details &cd)
{
const svalue *ptr_sval
= m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+ check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
cd.maybe_set_lhs (ptr_sval);
}
}
@@ -497,6 +499,7 @@ region_model::impl_call_malloc (const call_details &cd)
{
const svalue *ptr_sval
= m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+ check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
cd.maybe_set_lhs (ptr_sval);
}
}
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 7e7077696f7..1c23fb8c79e 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3303,7 +3303,8 @@ public:
m.add_cwe (131);
return warning_meta (rich_loc, m, get_controlling_option (),
- "allocated buffer size is not a multiple of the pointee's size");
+ "allocated buffer size is not a multiple"
+ " of the pointee's size");
}
label_text
@@ -3398,21 +3399,20 @@ capacity_compatible_with_type (tree cst, tree pointee_size_tree)
class size_visitor : public visitor
{
public:
- size_visitor (tree size_cst, const svalue *sval, constraint_manager *cm)
- : m_size_cst (size_cst), m_sval (sval), m_cm (cm)
+ size_visitor (tree size_cst, const svalue *root_sval, constraint_manager *cm)
+ : m_size_cst (size_cst), m_root_sval (root_sval), m_cm (cm)
{
- sval->accept (this);
+ m_root_sval->accept (this);
}
bool get_result ()
{
- return result_set.contains (m_sval);
+ return result_set.contains (m_root_sval);
}
void visit_constant_svalue (const constant_svalue *sval) final override
{
- if (capacity_compatible_with_type (sval->get_constant (), m_size_cst))
- result_set.add (sval);
+ check_constant (sval->get_constant (), sval);
}
void visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED)
@@ -3480,15 +3480,10 @@ public:
equiv_class_id id (-1);
if (m_cm->get_equiv_class_by_svalue (sval, &id))
{
- if (tree cst_val = id.get_obj (*m_cm).get_any_constant ())
- {
- if (capacity_compatible_with_type (cst_val, m_size_cst))
- result_set.add (sval);
- }
+ if (tree cst = id.get_obj (*m_cm).get_any_constant ())
+ check_constant (cst, sval);
else
- {
- result_set.add (sval);
- }
+ result_set.add (sval);
}
}
@@ -3505,8 +3500,23 @@ public:
}
private:
+ void check_constant (tree cst, const svalue *sval)
+ {
+ switch (TREE_CODE (cst))
+ {
+ default:
+ /* Assume all unhandled operands are compatible. */
+ result_set.add (sval);
+ break;
+ case INTEGER_CST:
+ if (capacity_compatible_with_type (cst, m_size_cst))
+ result_set.add (sval);
+ break;
+ }
+ }
+
tree m_size_cst;
- const svalue *m_sval;
+ const svalue *m_root_sval;
constraint_manager *m_cm;
svalue_set result_set; /* Used as a mapping of svalue*->bool. */
};
@@ -3543,12 +3553,12 @@ struct_or_union_with_inheritance_p (tree struc)
static bool
is_any_cast_p (const gimple *stmt)
{
- if (const gassign *assign = dyn_cast<const gassign *>(stmt))
+ if (const gassign *assign = dyn_cast <const gassign *> (stmt))
return gimple_assign_cast_p (assign)
|| !pending_diagnostic::same_tree_p (
TREE_TYPE (gimple_assign_lhs (assign)),
TREE_TYPE (gimple_assign_rhs1 (assign)));
- else if (const gcall *call = dyn_cast<const gcall *>(stmt))
+ else if (const gcall *call = dyn_cast <const gcall *> (stmt))
{
tree lhs = gimple_call_lhs (call);
return lhs != NULL_TREE && !pending_diagnostic::same_tree_p (
@@ -3608,10 +3618,11 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval,
case svalue_kind::SK_CONSTANT:
{
const constant_svalue *cst_cap_sval
- = as_a <const constant_svalue *> (capacity);
+ = as_a <const constant_svalue *> (capacity);
tree cst_cap = cst_cap_sval->get_constant ();
- if (!capacity_compatible_with_type (cst_cap, pointee_size_tree,
- is_struct))
+ if (TREE_CODE (cst_cap) == INTEGER_CST
+ && !capacity_compatible_with_type (cst_cap, pointee_size_tree,
+ is_struct))
ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg,
cst_cap));
}
@@ -3633,6 +3644,145 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval,
}
}
+/* Abstract class for diagnostics related touse of floating point
+ arithmetic where precision is needed. */
+
+class imprecise_floating_point_arithmetic
+: public pending_diagnostic_subclass<imprecise_floating_point_arithmetic>
+{
+public:
+ imprecise_floating_point_arithmetic ()
+ {}
+
+ const char *get_kind () const final override
+ {
+ return "imprecise_floating_point_arithmetic";
+ }
+
+ int get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_imprecise_floating_point_arithmetic;
+ }
+
+ bool
+ operator== (const imprecise_floating_point_arithmetic &other
+ ATTRIBUTE_UNUSED) const
+ {
+ return true;
+ }
+};
+
+/* Concrete diagnostic to complain about uses of floating point arithmetic
+ in the size argument of malloc etc. */
+
+class float_as_size_arg : public imprecise_floating_point_arithmetic
+{
+public:
+ float_as_size_arg (tree arg) : m_arg (arg)
+ {}
+
+ bool operator== (const float_as_size_arg &other) const
+ {
+ return pending_diagnostic::same_tree_p (m_arg, other.m_arg);
+ }
+
+ bool emit (rich_location *rich_loc) final override
+ {
+ diagnostic_metadata m;
+ bool warned = warning_meta (rich_loc, m, get_controlling_option (),
+ "use of floating point arithmetic inside the"
+ " size argument might yield unexpected"
+ " results");
+ if (warned)
+ inform (rich_loc->get_loc (), "only use operands of a type that"
+ " represents whole numbers inside the"
+ " size argument");
+ return warned;
+ }
+
+ label_text describe_final_event (const evdesc::final_event &ev) final
+ override
+ {
+ if (m_arg)
+ return ev.formatted_print ("operand %qE is of type %qT",
+ m_arg, TREE_TYPE (m_arg));
+ return ev.formatted_print ("at least one operand of the size argument is"
+ " of a floating point type");
+ }
+
+private:
+ tree m_arg;
+};
+
+/* Visitor to find uses of floating point variables/constants in an svalue. */
+
+class contains_floating_point_visitor : public visitor
+{
+public:
+ contains_floating_point_visitor (const svalue *root_sval) : m_result (NULL)
+ {
+ root_sval->accept (this);
+ }
+
+ const svalue *get_svalue_to_report ()
+ {
+ return m_result;
+ }
+
+ void visit_constant_svalue (const constant_svalue *sval) final override
+ {
+ /* At the point the analyzer runs, constant integer operands in a floating
+ point expression are already implictly converted to floating points.
+ Thus, we do prefer to report non-constants such that the diagnostic
+ always reports a floating point operand. */
+ tree type = sval->get_type ();
+ if (type && FLOAT_TYPE_P (type) && !m_result)
+ m_result = sval;
+ }
+
+ void visit_conjured_svalue (const conjured_svalue *sval) final override
+ {
+ tree type = sval->get_type ();
+ if (type && SCALAR_FLOAT_TYPE_P (type))
+ m_result = sval;
+ }
+
+ void visit_initial_svalue (const initial_svalue *sval) final override
+ {
+ tree type = sval->get_type ();
+ if (type && SCALAR_FLOAT_TYPE_P (type))
+ m_result = sval;
+ }
+
+private:
+ /* Non-null if at least one floating point operand was found. */
+ const svalue *m_result;
+};
+
+/* May complain about uses of floating point operands in the capacity of SVAL.
+
+ SVAL is expected to be a region_svalue. */
+
+void
+region_model::check_region_capacity_for_floats (const svalue *sval,
+ region_model_context *ctxt)
+const
+{
+ if (!ctxt)
+ return;
+
+ if (const region_svalue *reg_sval = dyn_cast <const region_svalue *> (sval))
+ {
+ const svalue *capacity = get_capacity (reg_sval->get_pointee ());
+ contains_floating_point_visitor v (capacity);
+ if (const svalue *float_sval = v.get_svalue_to_report ())
+ {
+ tree diag_arg = get_representative_tree (float_sval);
+ ctxt->warn (new float_as_size_arg (diag_arg));
+ }
+ }
+}
+
/* Set the value of the region given by LHS_REG to the value given
by RHS_SVAL.
Use CTXT to report any warnings associated with writing to LHS_REG. */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index cdf10872c0f..dcb87101733 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -869,6 +869,8 @@ class region_model
region_model_context *ctxt) const;
void check_region_size (const region *lhs_reg, const svalue *rhs_sval,
region_model_context *ctxt) const;
+ void check_region_capacity_for_floats (const svalue *reg_sval,
+ region_model_context *ctxt) const;
void check_region_bounds (const region *reg, enum access_direction dir,
region_model_context *ctxt) const;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index b264ae28fe6..4ef73a264ea 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -453,6 +453,7 @@ Objective-C and Objective-C++ Dialects}.
-Wno-analyzer-fd-use-without-check @gol
-Wno-analyzer-file-leak @gol
-Wno-analyzer-free-of-non-heap @gol
+-Wno-analyzer-imprecise-floating-point-arithmetic @gol
-Wno-analyzer-jump-through-null @gol
-Wno-analyzer-malloc-leak @gol
-Wno-analyzer-mismatching-deallocation @gol
@@ -9758,6 +9759,7 @@ Enabling this option effectively enables the following warnings:
-Wanalyzer-fd-use-without-check @gol
-Wanalyzer-file-leak @gol
-Wanalyzer-free-of-non-heap @gol
+-Wno-analyzer-imprecise-floating-point-arithmetic @gol
-Wanalyzer-jump-through-null @gol
-Wanalyzer-malloc-leak @gol
-Wanalyzer-mismatching-deallocation @gol
@@ -9946,6 +9948,18 @@ is called on a non-heap pointer (e.g. an on-stack buffer, or a global).
See @uref{https://cwe.mitre.org/data/definitions/590.html, CWE-590: Free of Memory not on the Heap}.
+@item -Wno-analyzer-imprecise-floating-point-arithmetic
+@opindex Wanalyzer-imprecise-floating-point-arithmetic
+@opindex Wno-analyzer-imprecise-floating-point-arithmetic
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-imprecise-floating-point-arithmetic}
+to disable it.
+
+This diagnostic warns for paths through the code in which floating point
+arithmetic is used in locations where precise computation is needed. This
+diagnostic only warns on use of floating points operands inside the
+allocation size at the moment.
+
@item -Wno-analyzer-jump-through-null
@opindex Wanalyzer-jump-through-null
@opindex Wno-analyzer-jump-through-null
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
index b5bed539250..003914ed96c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
@@ -115,3 +115,13 @@ void test_10 (int32_t n)
char *ptr = malloc (7 * n);
free (ptr);
}
+
+void test_11 ()
+{
+ /* 3.0 is folded to an int before the analyzer runs. */
+ int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */
+ free (ptr);
+
+ /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-* } malloc11 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
new file mode 100644
index 00000000000..110a9d3b9b3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
@@ -0,0 +1,62 @@
+#include <stdlib.h>
+
+/* Tests warn on use of floating point operands inside the allocation size.
+
+ The test cases here only test for warnings. The test cases inside
+ allocation-size-X.c should be plently enough to test for false positives. */
+
+void test_1 (float n)
+{
+ int *ptr = malloc (sizeof (int) * n); /* { dg-line test_1 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } test_1 } */
+ /* { dg-message "operand 'n' is of type 'float'" "note" { target *-*-* } test_1 } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } test_1 } */
+}
+
+void test_2 (int n)
+{
+ int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } test_2 } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_2 } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } test_2 } */
+}
+
+void *alloc_me (size_t size)
+{
+ return malloc (size); /* { dg-line test_3 } */
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } test_3 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_3 } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } test_3 } */
+}
+
+void test_3 (float f)
+{
+ void *ptr = alloc_me (f); /* { dg-message "calling 'alloc_me' from 'test_3'" } */
+ free (ptr);
+}
+
+void test_4 (int n)
+{
+ int *ptr = calloc(1.7 * n, sizeof (int)); /* { dg-line test_4 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } test_4 } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_4 } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } test_4 } */
+}
+
+int test_5 (float f)
+{
+ int *ptr = __builtin_alloca (sizeof (int) * f); /* { dg-line test_5 } */
+ *ptr = 4;
+ return *ptr;
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } test_5 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_5 } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } test_5 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106181.c b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
new file mode 100644
index 00000000000..af5b1cfdebc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
@@ -0,0 +1,11 @@
+#include <stdint.h>
+
+void *
+foo (int x)
+{
+ return __builtin_calloc (x * 1.1, 1); /* { dg-line calloc } */
+
+ /* { dg-warning "use of floating point arithmetic inside the size argument might yield unexpected results" "warning" { target *-*-* } calloc } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } calloc } */
+ /* { dg-message "only use operands of a type that represents whole numbers inside the size argument" "note" { target *-*-* } calloc } */
+}
--
2.36.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181]
2022-08-15 12:35 [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181] Tim Lange
@ 2022-08-15 18:16 ` David Malcolm
2022-08-18 9:44 ` [PATCH v2] analyzer: warn on the use of floating-points operands " Tim Lange
1 sibling, 0 replies; 4+ messages in thread
From: David Malcolm @ 2022-08-15 18:16 UTC (permalink / raw)
To: Tim Lange, gcc-patches
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.1000000000000001e+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 +98,10 @@ Wanalyzer-free-of-non-heap
> Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning
> Warn about code paths in which a non-heap pointer is freed.
>
> +Wanalyzer-imprecise-floating-point-arithmetic
> +Common Var(warn_analyzer_imprecise_floating_point_arithmetic)
> Init(1) Warning
> +Warn about code paths in which floating point arithmetic is use in
"use" -> "used".
> locations where precise computation is needed.
> +
> Wanalyzer-jump-through-null
> Common Var(warn_analyzer_jump_through_null) Init(1) Warning
> Warn about code paths in which a NULL function pointer is called.
> diff --git a/gcc/analyzer/region-model-impl-calls.cc
> b/gcc/analyzer/region-model-impl-calls.cc
> index 8eebd122d42..c4d7e186963 100644
> --- a/gcc/analyzer/region-model-impl-calls.cc
> +++ b/gcc/analyzer/region-model-impl-calls.cc
> @@ -237,6 +237,7 @@ region_model::impl_call_alloca (const
> call_details &cd)
> const region *new_reg = create_region_for_alloca (size_sval,
> cd.get_ctxt ());
> const svalue *ptr_sval
> = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> + check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
> cd.maybe_set_lhs (ptr_sval);
> }
>
> @@ -393,6 +394,7 @@ region_model::impl_call_calloc (const
> call_details &cd)
> {
> const svalue *ptr_sval
> = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> + check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
> cd.maybe_set_lhs (ptr_sval);
> }
> }
> @@ -497,6 +499,7 @@ region_model::impl_call_malloc (const
> call_details &cd)
> {
> const svalue *ptr_sval
> = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> + check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
> cd.maybe_set_lhs (ptr_sval);
> }
> }
These checks would be simpler if they were consolidated into a single
call to check_region_capacity_for_floats in:
region_model::set_dynamic_extents
akin to how we have check_dynamic_size_for_taint called there - though
that would make it harder to complain about specific arguments at
function calls. Given that we're not doing the latter, please
consolidate them.
[...snip...]
>
> +/* Abstract class for diagnostics related touse of floating point
"touse" -> "to use"
> + arithmetic where precision is needed. */
> +
> +class imprecise_floating_point_arithmetic
> +: public
> pending_diagnostic_subclass<imprecise_floating_point_arithmetic>
> +{
> +public:
> + imprecise_floating_point_arithmetic ()
> + {}
Do we need thic ctor? There aren't any fields to initialize.
> +
> + const char *get_kind () const final override
> + {
> + return "imprecise_floating_point_arithmetic";
> + }
get_kind should probably only be implemented for concrete subclasses,
such as float_as_size_arg.
> +
> + int get_controlling_option () const final override
> + {
> + return OPT_Wanalyzer_imprecise_floating_point_arithmetic;
> + }
> +
> + bool
> + operator== (const imprecise_floating_point_arithmetic &other
> + ATTRIBUTE_UNUSED) const
> + {
> + return true;
> + }
I don't think it makes sense to have an operator== here for an abstract
class with no fields.
> +};
> +
> +/* Concrete diagnostic to complain about uses of floating point
> arithmetic
> + in the size argument of malloc etc. */
> +
> +class float_as_size_arg : public imprecise_floating_point_arithmetic
> +{
> +public:
> + float_as_size_arg (tree arg) : m_arg (arg)
> + {}
> +
> + bool operator== (const float_as_size_arg &other) const
> + {
> + return pending_diagnostic::same_tree_p (m_arg, other.m_arg);
> + }
> +
> + bool emit (rich_location *rich_loc) final override
> + {
> + diagnostic_metadata m;
> + bool warned = warning_meta (rich_loc, m, get_controlling_option
> (),
> + "use of floating point arithmetic
> inside the"
> + " size argument might yield
> unexpected"
> + " results");
> + if (warned)
> + inform (rich_loc->get_loc (), "only use operands of a type
> that"
> + " represents whole numbers inside
> the"
> + " size argument");
> + return warned;
> + }
> +
> + label_text describe_final_event (const evdesc::final_event &ev)
> final
> + override
> + {
> + if (m_arg)
> + return ev.formatted_print ("operand %qE is of type %qT",
> + m_arg, TREE_TYPE (m_arg));
> + return ev.formatted_print ("at least one operand of the size
> argument is"
> + " of a floating point type");
> + }
> +
> +private:
> + tree m_arg;
> +};
> +
> +/* Visitor to find uses of floating point variables/constants in an
> svalue. */
> +
> +class contains_floating_point_visitor : public visitor
> +{
> +public:
> + contains_floating_point_visitor (const svalue *root_sval) :
> m_result (NULL)
> + {
> + root_sval->accept (this);
> + }
> +
> + const svalue *get_svalue_to_report ()
> + {
> + return m_result;
> + }
> +
> + void visit_constant_svalue (const constant_svalue *sval) final
> override
> + {
> + /* At the point the analyzer runs, constant integer operands in
> a floating
> + point expression are already implictly converted to floating
> points.
> + Thus, we do prefer to report non-constants such that the
> diagnostic
> + always reports a floating point operand. */
If the constant is seen first, then the non-constant won't be favored
(though perhaps binary ops get canonicalized so that constants are on
the RHS?).
Maybe rework this so that the visitor accumulates an auto_vec<const
svalue *> of all floating point svalues it sees, and have
get_svalue_to_report do a qsort of them (if non-empty), and pick the
first, using a custom comparator to order them into preference of
reporting? Though this is possibly overkill.
> + tree type = sval->get_type ();
> + if (type && FLOAT_TYPE_P (type) && !m_result)
> + m_result = sval;
> + }
> +
> + void visit_conjured_svalue (const conjured_svalue *sval) final
> override
> + {
> + tree type = sval->get_type ();
> + if (type && SCALAR_FLOAT_TYPE_P (type))
> + m_result = sval;
> + }
> +
> + void visit_initial_svalue (const initial_svalue *sval) final
> override
> + {
> + tree type = sval->get_type ();
> + if (type && SCALAR_FLOAT_TYPE_P (type))
> + m_result = sval;
> + }
> +
> +private:
> + /* Non-null if at least one floating point operand was found. */
> + const svalue *m_result;
> +};
> +
> +/* May complain about uses of floating point operands in the
> capacity of SVAL.
> +
> + SVAL is expected to be a region_svalue. */
> +
> +void
> +region_model::check_region_capacity_for_floats (const svalue *sval,
> + region_model_context
> *ctxt)
> +const
> +{
> + if (!ctxt)
> + return;
> +
> + if (const region_svalue *reg_sval = dyn_cast <const region_svalue
> *> (sval))
> + {
> + const svalue *capacity = get_capacity (reg_sval->get_pointee
> ());
> + contains_floating_point_visitor v (capacity);
> + if (const svalue *float_sval = v.get_svalue_to_report ())
> + {
> + tree diag_arg = get_representative_tree (float_sval);
> + ctxt->warn (new float_as_size_arg (diag_arg));
> + }
> + }
> +}
> +
> /* Set the value of the region given by LHS_REG to the value given
> by RHS_SVAL.
> Use CTXT to report any warnings associated with writing to
> LHS_REG. */
[...snip...]
> @@ -9758,6 +9759,7 @@ Enabling this option effectively enables the
> following warnings:
> -Wanalyzer-fd-use-without-check @gol
> -Wanalyzer-file-leak @gol
> -Wanalyzer-free-of-non-heap @gol
> +-Wno-analyzer-imprecise-floating-point-arithmetic @gol
Lose the "no-" here.
> -Wanalyzer-jump-through-null @gol
> -Wanalyzer-malloc-leak @gol
> -Wanalyzer-mismatching-deallocation @gol
> @@ -9946,6 +9948,18 @@ is called on a non-heap pointer (e.g. an on-
> stack buffer, or a global).
>
> See @uref{https://cwe.mitre.org/data/definitions/590.html, CWE-590:
> Free of Memory not on the Heap}.
>
> +@item -Wno-analyzer-imprecise-floating-point-arithmetic
> +@opindex Wanalyzer-imprecise-floating-point-arithmetic
> +@opindex Wno-analyzer-imprecise-floating-point-arithmetic
> +This warning requires @option{-fanalyzer}, which enables it; use
> +@option{-Wno-analyzer-imprecise-floating-point-arithmetic}
> +to disable it.
> +
> +This diagnostic warns for paths through the code in which floating
> point
> +arithmetic is used in locations where precise computation is
> needed. This
> +diagnostic only warns on use of floating points operands inside the
> +allocation size at the moment.
"inside the allocation size " -> "inside the calculation of an
allocation size".
[...snip...]
Other than the above, the patch looks good; thanks.
Dave
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181]
2022-08-15 12:35 [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181] Tim Lange
2022-08-15 18:16 ` David Malcolm
@ 2022-08-18 9:44 ` Tim Lange
2022-08-18 12:21 ` David Malcolm
1 sibling, 1 reply; 4+ messages in thread
From: Tim Lange @ 2022-08-18 9:44 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, Tim Lange
Hi,
this is the revised version of my patch. I had trouble to get your
point regarding the float_visitor:
> If the constant is seen first, then the non-constant won't be favored
> (though perhaps binary ops get canonicalized so that constants are on
> the RHS?).
Only the assignment of m_result in visit_constant_svalue is guarded by
!m_result, while the other two are not. So, there are two possibilities:
1. A constant is seen first and then assigned to m_result.
1.1. A non-constant float operand is seen later and
overwrites m_result.
1.2. There's no non-constant float operand, thus the
constant is the actual floating-point operand and
is kept inside m_result.
2. A non-constant is seen first, then m_result might be
overwritten with another non-constant later but never
with a constant.
Do I have a flaw in my thinking? (But they do seem to get canonicalized,
so that shouldn't matter)
> How about:
> -Wanalyzer-imprecise-float-arithmetic
> -Wanalyzer-imprecise-fp-arithmetic
> instead? (ideas welcome)
I've chosen the second. I mostly tried to avoid float because it is also
a reserved keyword in many languages and I wanted to avoid confusion
(might be overthinking that).
- Tim
This patch fixes the ICE reported in PR106181 and adds a new warning to
the analyzer complaining about the use of floating-point operands.
Regrtested on Linux x86_64.
2022-08-17 Tim Lange <mail@tim-lange.me>
gcc/analyzer/ChangeLog:
PR analyzer/106181
* analyzer.opt: Add Wanalyzer-imprecise-floating-point-arithmetic.
* 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_dynamic_size_for_floats): New function.
(region_model::set_dynamic_extents):
Call to check_dynamic_size_for_floats.
* region-model.h (class region_model):
Add region_model::check_dynamic_size_for_floats.
gcc/ChangeLog:
PR analyzer/106181
* doc/invoke.texi: Add Wanalyzer-imprecise-fp-arithmetic.
gcc/testsuite/ChangeLog:
PR analyzer/106181
* gcc.dg/analyzer/allocation-size-1.c: New test.
* gcc.dg/analyzer/imprecise-floating-point-1.c: New test.
* gcc.dg/analyzer/pr106181.c: New test.
---
gcc/analyzer/analyzer.opt | 4 +
gcc/analyzer/region-model.cc | 179 +++++++++++++++---
gcc/analyzer/region-model.h | 2 +
gcc/doc/invoke.texi | 14 ++
.../gcc.dg/analyzer/allocation-size-1.c | 10 +
.../analyzer/imprecise-floating-point-1.c | 74 ++++++++
gcc/testsuite/gcc.dg/analyzer/pr106181.c | 11 ++
7 files changed, 271 insertions(+), 23 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 61b58c575ff..437ea92e130 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -98,6 +98,10 @@ Wanalyzer-free-of-non-heap
Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning
Warn about code paths in which a non-heap pointer is freed.
+Wanalyzer-imprecise-fp-arithmetic
+Common Var(warn_analyzer_imprecise_fp_arithmetic) Init(1) Warning
+Warn about code paths in which floating-point arithmetic is used in locations where precise computation is needed.
+
Wanalyzer-jump-through-null
Common Var(warn_analyzer_jump_through_null) Init(1) Warning
Warn about code paths in which a NULL function pointer is called.
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index b5bc3efda32..ec29be259b5 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3301,7 +3301,8 @@ public:
m.add_cwe (131);
return warning_meta (rich_loc, m, get_controlling_option (),
- "allocated buffer size is not a multiple of the pointee's size");
+ "allocated buffer size is not a multiple"
+ " of the pointee's size");
}
label_text
@@ -3396,21 +3397,20 @@ capacity_compatible_with_type (tree cst, tree pointee_size_tree)
class size_visitor : public visitor
{
public:
- size_visitor (tree size_cst, const svalue *sval, constraint_manager *cm)
- : m_size_cst (size_cst), m_sval (sval), m_cm (cm)
+ size_visitor (tree size_cst, const svalue *root_sval, constraint_manager *cm)
+ : m_size_cst (size_cst), m_root_sval (root_sval), m_cm (cm)
{
- sval->accept (this);
+ m_root_sval->accept (this);
}
bool get_result ()
{
- return result_set.contains (m_sval);
+ return result_set.contains (m_root_sval);
}
void visit_constant_svalue (const constant_svalue *sval) final override
{
- if (capacity_compatible_with_type (sval->get_constant (), m_size_cst))
- result_set.add (sval);
+ check_constant (sval->get_constant (), sval);
}
void visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED)
@@ -3478,15 +3478,10 @@ public:
equiv_class_id id (-1);
if (m_cm->get_equiv_class_by_svalue (sval, &id))
{
- if (tree cst_val = id.get_obj (*m_cm).get_any_constant ())
- {
- if (capacity_compatible_with_type (cst_val, m_size_cst))
- result_set.add (sval);
- }
+ if (tree cst = id.get_obj (*m_cm).get_any_constant ())
+ check_constant (cst, sval);
else
- {
- result_set.add (sval);
- }
+ result_set.add (sval);
}
}
@@ -3503,8 +3498,23 @@ public:
}
private:
+ void check_constant (tree cst, const svalue *sval)
+ {
+ switch (TREE_CODE (cst))
+ {
+ default:
+ /* Assume all unhandled operands are compatible. */
+ result_set.add (sval);
+ break;
+ case INTEGER_CST:
+ if (capacity_compatible_with_type (cst, m_size_cst))
+ result_set.add (sval);
+ break;
+ }
+ }
+
tree m_size_cst;
- const svalue *m_sval;
+ const svalue *m_root_sval;
constraint_manager *m_cm;
svalue_set result_set; /* Used as a mapping of svalue*->bool. */
};
@@ -3541,12 +3551,12 @@ struct_or_union_with_inheritance_p (tree struc)
static bool
is_any_cast_p (const gimple *stmt)
{
- if (const gassign *assign = dyn_cast<const gassign *>(stmt))
+ if (const gassign *assign = dyn_cast <const gassign *> (stmt))
return gimple_assign_cast_p (assign)
|| !pending_diagnostic::same_tree_p (
TREE_TYPE (gimple_assign_lhs (assign)),
TREE_TYPE (gimple_assign_rhs1 (assign)));
- else if (const gcall *call = dyn_cast<const gcall *>(stmt))
+ else if (const gcall *call = dyn_cast <const gcall *> (stmt))
{
tree lhs = gimple_call_lhs (call);
return lhs != NULL_TREE && !pending_diagnostic::same_tree_p (
@@ -3606,10 +3616,11 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval,
case svalue_kind::SK_CONSTANT:
{
const constant_svalue *cst_cap_sval
- = as_a <const constant_svalue *> (capacity);
+ = as_a <const constant_svalue *> (capacity);
tree cst_cap = cst_cap_sval->get_constant ();
- if (!capacity_compatible_with_type (cst_cap, pointee_size_tree,
- is_struct))
+ if (TREE_CODE (cst_cap) == INTEGER_CST
+ && !capacity_compatible_with_type (cst_cap, pointee_size_tree,
+ is_struct))
ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg,
cst_cap));
}
@@ -5055,6 +5066,125 @@ region_model::append_regions_cb (const region *base_reg,
cb_data->out->safe_push (decl_reg);
}
+
+/* Abstract class for diagnostics related to the use of
+ floating-point arithmetic where precision is needed. */
+
+class imprecise_floating_point_arithmetic : public pending_diagnostic
+{
+public:
+ int get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_imprecise_fp_arithmetic;
+ }
+};
+
+/* Concrete diagnostic to complain about uses of floating-point arithmetic
+ in the size argument of malloc etc. */
+
+class float_as_size_arg : public imprecise_floating_point_arithmetic
+{
+public:
+ float_as_size_arg (tree arg) : m_arg (arg)
+ {}
+
+ const char *get_kind () const final override
+ {
+ return "float_as_size_arg_diagnostic";
+ }
+
+ bool subclass_equal_p (const pending_diagnostic &other) const
+ {
+ return same_tree_p (m_arg, ((const float_as_size_arg &) other).m_arg);
+ }
+
+ bool emit (rich_location *rich_loc) final override
+ {
+ diagnostic_metadata m;
+ bool warned = warning_meta (rich_loc, m, get_controlling_option (),
+ "use of floating-point arithmetic here might"
+ " yield unexpected results");
+ if (warned)
+ inform (rich_loc->get_loc (), "only use operands of an integer type"
+ " inside the size argument");
+ return warned;
+ }
+
+ label_text describe_final_event (const evdesc::final_event &ev) final
+ override
+ {
+ if (m_arg)
+ return ev.formatted_print ("operand %qE is of type %qT",
+ m_arg, TREE_TYPE (m_arg));
+ return ev.formatted_print ("at least one operand of the size argument is"
+ " of a floating-point type");
+ }
+
+private:
+ tree m_arg;
+};
+
+/* Visitor to find uses of floating-point variables/constants in an svalue. */
+
+class contains_floating_point_visitor : public visitor
+{
+public:
+ contains_floating_point_visitor (const svalue *root_sval) : m_result (NULL)
+ {
+ root_sval->accept (this);
+ }
+
+ const svalue *get_svalue_to_report ()
+ {
+ return m_result;
+ }
+
+ void visit_constant_svalue (const constant_svalue *sval) final override
+ {
+ /* At the point the analyzer runs, constant integer operands in a floating
+ point expression are already implictly converted to floating-points.
+ Thus, we do prefer to report non-constants such that the diagnostic
+ always reports a floating-point operand. */
+ tree type = sval->get_type ();
+ if (type && FLOAT_TYPE_P (type) && !m_result)
+ m_result = sval;
+ }
+
+ void visit_conjured_svalue (const conjured_svalue *sval) final override
+ {
+ tree type = sval->get_type ();
+ if (type && FLOAT_TYPE_P (type))
+ m_result = sval;
+ }
+
+ void visit_initial_svalue (const initial_svalue *sval) final override
+ {
+ tree type = sval->get_type ();
+ if (type && FLOAT_TYPE_P (type))
+ m_result = sval;
+ }
+
+private:
+ /* Non-null if at least one floating-point operand was found. */
+ const svalue *m_result;
+};
+
+/* May complain about uses of floating-point operands in SIZE_IN_BYTES. */
+
+void
+region_model::check_dynamic_size_for_floats (const svalue *size_in_bytes,
+ region_model_context *ctxt) const
+{
+ gcc_assert (ctxt);
+
+ contains_floating_point_visitor v (size_in_bytes);
+ if (const svalue *float_sval = v.get_svalue_to_report ())
+ {
+ tree diag_arg = get_representative_tree (float_sval);
+ ctxt->warn (new float_as_size_arg (diag_arg));
+ }
+}
+
/* Return a new region describing a heap-allocated block of memory.
Use CTXT to complain about tainted sizes. */
@@ -5092,8 +5222,11 @@ region_model::set_dynamic_extents (const region *reg,
{
assert_compat_types (size_in_bytes->get_type (), size_type_node);
if (ctxt)
- check_dynamic_size_for_taint (reg->get_memory_space (), size_in_bytes,
- ctxt);
+ {
+ check_dynamic_size_for_taint (reg->get_memory_space (), size_in_bytes,
+ ctxt);
+ check_dynamic_size_for_floats (size_in_bytes, ctxt);
+ }
m_dynamic_extents.put (reg, size_in_bytes);
}
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index cdf10872c0f..7ce832f6ce4 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -853,6 +853,8 @@ class region_model
void check_dynamic_size_for_taint (enum memory_space mem_space,
const svalue *size_in_bytes,
region_model_context *ctxt) const;
+ void check_dynamic_size_for_floats (const svalue *size_in_bytes,
+ region_model_context *ctxt) const;
void check_region_for_taint (const region *reg,
enum access_direction dir,
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 1ac81ad0bb4..f65d351a5fc 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -453,6 +453,7 @@ Objective-C and Objective-C++ Dialects}.
-Wno-analyzer-fd-use-without-check @gol
-Wno-analyzer-file-leak @gol
-Wno-analyzer-free-of-non-heap @gol
+-Wno-analyzer-imprecise-fp-arithmetic @gol
-Wno-analyzer-jump-through-null @gol
-Wno-analyzer-malloc-leak @gol
-Wno-analyzer-mismatching-deallocation @gol
@@ -9758,6 +9759,7 @@ Enabling this option effectively enables the following warnings:
-Wanalyzer-fd-use-without-check @gol
-Wanalyzer-file-leak @gol
-Wanalyzer-free-of-non-heap @gol
+-Wanalyzer-imprecise-fp-arithmetic @gol
-Wanalyzer-jump-through-null @gol
-Wanalyzer-malloc-leak @gol
-Wanalyzer-mismatching-deallocation @gol
@@ -9946,6 +9948,18 @@ is called on a non-heap pointer (e.g. an on-stack buffer, or a global).
See @uref{https://cwe.mitre.org/data/definitions/590.html, CWE-590: Free of Memory not on the Heap}.
+@item -Wno-analyzer-imprecise-fp-arithmetic
+@opindex Wanalyzer-imprecise-fp-arithmetic
+@opindex Wno-analyzer-imprecise-fp-arithmetic
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-imprecise-fp-arithmetic}
+to disable it.
+
+This diagnostic warns for paths through the code in which floating-point
+arithmetic is used in locations where precise computation is needed. This
+diagnostic only warns on use of floating-point operands inside the
+calculation of an allocation size at the moment.
+
@item -Wno-analyzer-jump-through-null
@opindex Wanalyzer-jump-through-null
@opindex Wno-analyzer-jump-through-null
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
index b5bed539250..003914ed96c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
@@ -115,3 +115,13 @@ void test_10 (int32_t n)
char *ptr = malloc (7 * n);
free (ptr);
}
+
+void test_11 ()
+{
+ /* 3.0 is folded to an int before the analyzer runs. */
+ int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */
+ free (ptr);
+
+ /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 } */
+ /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-* } malloc11 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
new file mode 100644
index 00000000000..d8a3f4884d6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c
@@ -0,0 +1,74 @@
+#include <stdlib.h>
+
+/* Tests warn on use of floating-point operands inside the calculation
+ of an allocation size.
+
+ The test cases here only test for warnings. The test cases inside
+ allocation-size-X.c should be plently enough to test for false positives. */
+
+void test_1 (float f)
+{
+ int *ptr = malloc (sizeof (int) * f); /* { dg-line test_1 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_1 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_1 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_1 } */
+}
+
+void test_2 (int n)
+{
+ int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_2 } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_2 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_2 } */
+}
+
+void *alloc_me (size_t size)
+{
+ return malloc (size); /* { dg-line test_3 } */
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_3 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_3 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_3 } */
+}
+
+void test_3 (float f)
+{
+ void *ptr = alloc_me (f); /* { dg-message "calling 'alloc_me' from 'test_3'" } */
+ free (ptr);
+}
+
+void test_4 (int n)
+{
+ int *ptr = calloc(1.7 * n, sizeof (int)); /* { dg-line test_4 } */
+ free (ptr);
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_4 } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_4 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_4 } */
+}
+
+int test_5 (float f)
+{
+ int *ptr = __builtin_alloca (sizeof (int) * f); /* { dg-line test_5 } */
+ *ptr = 4;
+ return *ptr;
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_5 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_5 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_5 } */
+}
+
+int test_6 (float f)
+{
+ int *ptr = __builtin_alloca (1.7f * f * 2.3f); /* { dg-line test_6 } */
+ *ptr = 4;
+ return *ptr;
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_6 } */
+ /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_6 } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_6 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106181.c b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
new file mode 100644
index 00000000000..6a78b78d352
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/pr106181.c
@@ -0,0 +1,11 @@
+#include <stdint.h>
+
+void *
+foo (int x)
+{
+ return __builtin_calloc (x * 1.1, 1); /* { dg-line calloc } */
+
+ /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } calloc } */
+ /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } calloc } */
+ /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } calloc } */
+}
--
2.37.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181]
2022-08-18 9:44 ` [PATCH v2] analyzer: warn on the use of floating-points operands " Tim Lange
@ 2022-08-18 12:21 ` David Malcolm
0 siblings, 0 replies; 4+ messages in thread
From: David Malcolm @ 2022-08-18 12:21 UTC (permalink / raw)
To: Tim Lange, gcc-patches
On Thu, 2022-08-18 at 11:44 +0200, Tim Lange wrote:
> Hi,
>
> this is the revised version of my patch. I had trouble to get your
> point regarding the float_visitor:
>
> > If the constant is seen first, then the non-constant won't be
> > favored
> > (though perhaps binary ops get canonicalized so that constants are
> > on
> > the RHS?).
>
> Only the assignment of m_result in visit_constant_svalue is guarded
> by
> !m_result, while the other two are not. So, there are two
> possibilities:
> 1. A constant is seen first and then assigned to m_result.
> 1.1. A non-constant float operand is seen later and
> overwrites m_result.
> 1.2. There's no non-constant float operand, thus the
> constant is the actual floating-point operand
> and
> is kept inside m_result.
> 2. A non-constant is seen first, then m_result might be
> overwritten with another non-constant later but never
> with a constant.
> Do I have a flaw in my thinking? (But they do seem to get
> canonicalized,
> so that shouldn't matter)
I think I was confused here, and that you're right. Sorry about that.
>
> > How about:
> > -Wanalyzer-imprecise-float-arithmetic
> > -Wanalyzer-imprecise-fp-arithmetic
> > instead? (ideas welcome)
>
> I've chosen the second. I mostly tried to avoid float because it is
> also
> a reserved keyword in many languages and I wanted to avoid confusion
> (might be overthinking that).
Fair enough.
>
> - Tim
>
> This patch fixes the ICE reported in PR106181 and adds a new warning
> to
> the analyzer complaining about the use of floating-point operands.
>
> Regrtested on Linux x86_64.
Thanks; the patch looks good for trunk.
Dave
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-18 12:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-15 12:35 [PATCH] analyzer: warn on the use of floating points in the size argument [PR106181] Tim Lange
2022-08-15 18:16 ` David Malcolm
2022-08-18 9:44 ` [PATCH v2] analyzer: warn on the use of floating-points operands " Tim Lange
2022-08-18 12:21 ` David Malcolm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).