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
@@ -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.
@@ -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);
}
@@ -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,
@@ -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
@@ -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 } */
+}
new file mode 100644
@@ -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 } */
+}
new file mode 100644
@@ -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 } */
+}