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

[David Malcolm <dmalcolm at redhat dot com>]

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