[committed] analyzer: avoid comparisons between uncomparable types (PR 93450)

Message ID 20200130142733.29915-1-dmalcolm@redhat.com
State New
Headers show
Series
  • [committed] analyzer: avoid comparisons between uncomparable types (PR 93450)
Related show

Commit Message

David Malcolm Jan. 30, 2020, 2:27 p.m.
PR analyzer/93450 reports an ICE trying to fold an EQ_EXPR comparison
of (int)0 with (float)Inf.

Most comparisons inside the analyzer come from gimple conditions, for
which the necessary casts have already been added.

This one is done inside constant_svalue::eval_condition as part of
purging sm-state for an unknown function call, and fails to check
the types being compared, leading to the ICE.

sm_state_map::set_state calls region_model::eval_condition_without_cm in
order to handle pointer equality (so that e.g. (void *)&r and (foo *)&r
transition together), which leads to this code generating a bogus query
to see if the two constants are equal.

This patch fixes the ICE in two ways:

- It avoids generating comparisons within
  constant_svalue::eval_condition unless the types are equal (thus for
  constants, but not for pointer values, which are handled by
  region_svalue).

- It updates sm_state_map::set_state to bail immediately if the new
  state is the same as the old one, thus avoiding the above for the
  common case where an svalue_id has no sm-state (such as for the int
  and float constants in the reproducer), for which the above becomes a
  no-op.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Committed to master as r10-6351-gd177c49cd31131c8cededb216da30877d8a3856d.

gcc/analyzer/ChangeLog:
	PR analyzer/93450
	* program-state.cc (sm_state_map::set_state): For the overload
	taking an svalue_id, bail out if the set_state on the ec does
	nothing.  Convert the latter's return type from void to bool,
	returning true if anything changed.
	(sm_state_map::impl_set_state): Convert the return type from void
	to bool, returning true if the state changed.
	* program-state.h (sm_state_map::set_state): Convert return type
	from void to bool.
	(sm_state_map::impl_set_state): Likewise.
	* region-model.cc (constant_svalue::eval_condition): Only call
	fold_build2 if the types are the same.

gcc/testsuite/ChangeLog:
	PR analyzer/93450
	* gcc.dg/analyzer/torture/pr93450.c: New test.
---
 gcc/analyzer/program-state.cc                 | 23 +++++++++++------
 gcc/analyzer/program-state.h                  |  4 +--
 gcc/analyzer/region-model.cc                  | 16 +++++++-----
 .../gcc.dg/analyzer/torture/pr93450.c         | 25 +++++++++++++++++++
 4 files changed, 53 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/torture/pr93450.c

-- 
2.21.0

Comments

Jakub Jelinek Jan. 30, 2020, 2:36 p.m. | #1
On Thu, Jan 30, 2020 at 09:27:33AM -0500, David Malcolm wrote:
> --- a/gcc/analyzer/region-model.cc

> +++ b/gcc/analyzer/region-model.cc

> @@ -666,12 +666,16 @@ constant_svalue::eval_condition (constant_svalue *lhs,

>    gcc_assert (CONSTANT_CLASS_P (lhs_const));

>    gcc_assert (CONSTANT_CLASS_P (rhs_const));

>  

> -  tree comparison

> -    = fold_build2 (op, boolean_type_node, lhs_const, rhs_const);

> -  if (comparison == boolean_true_node)

> -    return tristate (tristate::TS_TRUE);

> -  if (comparison == boolean_false_node)

> -    return tristate (tristate::TS_FALSE);

> +  /* Check for comparable types.  */

> +  if (TREE_TYPE (lhs_const) == TREE_TYPE (rhs_const))


Isn't the analyzer invoked on GIMPLE?  In GIMPLE, pointer equality of types
is often not guaranteed and the compiler generally doesn't care about that,
what we care about is whether the two types are compatible
(types_compatible_p).  E.g. if originally both types were say long int,
but on lp64 one of the arguments was cast from long long int to long int,
you can end up with one operand long int and the other long long int;
they have the same precision etc., so it is ok to compare them.

> +    {

> +      tree comparison

> +	= fold_build2 (op, boolean_type_node, lhs_const, rhs_const);

> +      if (comparison == boolean_true_node)

> +	return tristate (tristate::TS_TRUE);

> +      if (comparison == boolean_false_node)

> +	return tristate (tristate::TS_FALSE);


This seems to be a waste of compile time memory.  fold_build2 is essentially
  tem = fold_binary_loc (loc, code, type, op0, op1);
  if (!tem)
    tem = build2_loc (loc, code, type, op0, op1 PASS_MEM_STAT);
but as you only care if the result is boolean_true_node or
boolean_false_node, the build2_loc is unnecessary.  So, just use
fold_binary instead?  If it returns NULL_TREE, it won't compare equal to
either and will fall through to the TS_UNKNOWN case.
> +    }

>    return tristate::TS_UNKNOWN;

>  }


	Jakub
David Malcolm Jan. 31, 2020, 1:19 a.m. | #2
On Thu, 2020-01-30 at 15:36 +0100, Jakub Jelinek wrote:
> On Thu, Jan 30, 2020 at 09:27:33AM -0500, David Malcolm wrote:

> > --- a/gcc/analyzer/region-model.cc

> > +++ b/gcc/analyzer/region-model.cc

> > @@ -666,12 +666,16 @@ constant_svalue::eval_condition

> > (constant_svalue *lhs,

> >    gcc_assert (CONSTANT_CLASS_P (lhs_const));

> >    gcc_assert (CONSTANT_CLASS_P (rhs_const));

> >  

> > -  tree comparison

> > -    = fold_build2 (op, boolean_type_node, lhs_const, rhs_const);

> > -  if (comparison == boolean_true_node)

> > -    return tristate (tristate::TS_TRUE);

> > -  if (comparison == boolean_false_node)

> > -    return tristate (tristate::TS_FALSE);

> > +  /* Check for comparable types.  */

> > +  if (TREE_TYPE (lhs_const) == TREE_TYPE (rhs_const))

> 

> Isn't the analyzer invoked on GIMPLE?  In GIMPLE, pointer equality of

> types

> is often not guaranteed and the compiler generally doesn't care about

> that,

> what we care about is whether the two types are compatible

> (types_compatible_p).  E.g. if originally both types were say long

> int,

> but on lp64 one of the arguments was cast from long long int to long

> int,

> you can end up with one operand long int and the other long long int;

> they have the same precision etc., so it is ok to compare them.


Thanks.  In patch 1 of the attached I've replaced the pointer comparison
with a call to types_compatible_p, and added a call to
types_compatible_p to guard a place where constants were being compared.

> > +    {

> > +      tree comparison

> > +	= fold_build2 (op, boolean_type_node, lhs_const, rhs_const);

> > +      if (comparison == boolean_true_node)

> > +	return tristate (tristate::TS_TRUE);

> > +      if (comparison == boolean_false_node)

> > +	return tristate (tristate::TS_FALSE);

> 

> This seems to be a waste of compile time memory.  fold_build2 is

> essentially

>   tem = fold_binary_loc (loc, code, type, op0, op1);

>   if (!tem)

>     tem = build2_loc (loc, code, type, op0, op1 PASS_MEM_STAT);

> but as you only care if the result is boolean_true_node or

> boolean_false_node, the build2_loc is unnecessary.  So, just use

> fold_binary instead?  If it returns NULL_TREE, it won't compare equal

> to

> either and will fall through to the TS_UNKNOWN case.


Thanks.  I did some grepping and found that I'd made this mistake in
six places in the analyzer.  Sorry about that.  Patch 2 should fix all
of them.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for master?

David Malcolm (2):
  analyzer: further fixes for comparisons between uncomparable types (PR
    93450)
  analyzer: avoid use of fold_build2

 gcc/analyzer/constraint-manager.cc | 19 +++++++++----------
 gcc/analyzer/region-model.cc       |  8 ++++----
 2 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.21.0

Patch

diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index a9e300fba0f..f41f105ce6b 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -259,7 +259,8 @@  sm_state_map::set_state (region_model *model,
   if (model == NULL)
     return;
   equiv_class &ec = model->get_constraints ()->get_equiv_class (sid);
-  set_state (ec, state, origin);
+  if (!set_state (ec, state, origin))
+    return;
 
   /* Also do it for all svalues that are equal via non-cm, so that
      e.g. (void *)&r and (foo *)&r transition together.  */
@@ -276,34 +277,42 @@  sm_state_map::set_state (region_model *model,
 }
 
 /* Set the state of EC to STATE, recording that the state came from
-   ORIGIN.  */
+   ORIGIN.
+   Return true if any states of svalue_ids within EC changed.  */
 
-void
+bool
 sm_state_map::set_state (const equiv_class &ec,
 			 state_machine::state_t state,
 			 svalue_id origin)
 {
   int i;
   svalue_id *sid;
+  bool any_changed = false;
   FOR_EACH_VEC_ELT (ec.m_vars, i, sid)
-    impl_set_state (*sid, state, origin);
+    any_changed |= impl_set_state (*sid, state, origin);
+  return any_changed;
 }
 
-/* Set state of PV to STATE, bypassing equivalence classes.  */
+/* Set state of SID to STATE, bypassing equivalence classes.
+   Return true if the state changed.  */
 
-void
+bool
 sm_state_map::impl_set_state (svalue_id sid, state_machine::state_t state,
 			      svalue_id origin)
 {
+  if (get_state (sid) == state)
+    return false;
+
   /* Special-case state 0 as the default value.  */
   if (state == 0)
     {
       if (m_map.get (sid))
 	m_map.remove (sid);
-      return;
+      return true;
     }
   gcc_assert (!sid.null_p ());
   m_map.put (sid, entry_t (state, origin));
+  return true;
 }
 
 /* Set the "global" state within this state map to STATE.  */
diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h
index adc71a4eda2..0a4e35f3d5d 100644
--- a/gcc/analyzer/program-state.h
+++ b/gcc/analyzer/program-state.h
@@ -161,10 +161,10 @@  public:
 		  svalue_id sid,
 		  state_machine::state_t state,
 		  svalue_id origin);
-  void set_state (const equiv_class &ec,
+  bool set_state (const equiv_class &ec,
 		  state_machine::state_t state,
 		  svalue_id origin);
-  void impl_set_state (svalue_id sid,
+  bool impl_set_state (svalue_id sid,
 		       state_machine::state_t state,
 		       svalue_id origin);
 
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index a5b3dffcc27..c838454a1eb 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -666,12 +666,16 @@  constant_svalue::eval_condition (constant_svalue *lhs,
   gcc_assert (CONSTANT_CLASS_P (lhs_const));
   gcc_assert (CONSTANT_CLASS_P (rhs_const));
 
-  tree comparison
-    = fold_build2 (op, boolean_type_node, lhs_const, rhs_const);
-  if (comparison == boolean_true_node)
-    return tristate (tristate::TS_TRUE);
-  if (comparison == boolean_false_node)
-    return tristate (tristate::TS_FALSE);
+  /* Check for comparable types.  */
+  if (TREE_TYPE (lhs_const) == TREE_TYPE (rhs_const))
+    {
+      tree comparison
+	= fold_build2 (op, boolean_type_node, lhs_const, rhs_const);
+      if (comparison == boolean_true_node)
+	return tristate (tristate::TS_TRUE);
+      if (comparison == boolean_false_node)
+	return tristate (tristate::TS_FALSE);
+    }
   return tristate::TS_UNKNOWN;
 }
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/pr93450.c b/gcc/testsuite/gcc.dg/analyzer/torture/pr93450.c
new file mode 100644
index 00000000000..7f6cba0db6d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/torture/pr93450.c
@@ -0,0 +1,25 @@ 
+void
+ed (int);
+
+double
+bg (void)
+{
+  double kl = __builtin_huge_val ();
+
+  ed (0);
+
+  return kl;
+}
+
+static double __attribute__((noinline))
+get_hugeval (void)
+{
+  return __builtin_huge_val ();
+}
+
+int test_2 (int i)
+{
+  if (i < get_hugeval ())
+    return 42;
+  return 0;
+}