Re: [PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181]

2022-08-18 Thread David Malcolm via 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



[PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181]

2022-08-18 Thread 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  

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 (roo