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

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