analyzer: fix handling of negative byte offsets (v2) (PR 93281)

Message ID 20200117023645.4994-1-dmalcolm@redhat.com
State New
Headers show
Series
  • analyzer: fix handling of negative byte offsets (v2) (PR 93281)
Related show

Commit Message

David Malcolm Jan. 17, 2020, 2:36 a.m.
On Thu, 2020-01-16 at 09:14 +0100, Jakub Jelinek wrote:
> On Wed, Jan 15, 2020 at 06:56:48PM -0500, David Malcolm wrote:

> > gcc/analyzer/ChangeLog:

> > 	PR analyzer/93281

> > 	* region-model.cc

> > 	(region_model::convert_byte_offset_to_array_index): Convert

> > 	offset_cst to ssizetype before dividing by byte_size.

> > ---

> >  gcc/analyzer/region-model.cc | 5 ++++-

> >  1 file changed, 4 insertions(+), 1 deletion(-)

> > 

> > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-

> > model.cc

> > index 5c39be4fd7f..b62ddf82a40 100644

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

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

> > @@ -6414,9 +6414,12 @@

> > region_model::convert_byte_offset_to_array_index (tree ptr_type,

> >        /* This might not be a constant.  */

> >        tree byte_size = size_in_bytes (elem_type);

> >  

> > +      /* Ensure we're in a signed representation before doing the

> > division.  */

> > +      tree signed_offset_cst = fold_convert (ssizetype,

> > offset_cst);

> > +

> >        tree index

> >  	= fold_build2 (TRUNC_DIV_EXPR, integer_type_node,

> > -		       offset_cst, byte_size);

> > +		       signed_offset_cst, byte_size);

> 

> I'd say you want to fold_convert byte_size to ssizetype too.

> Another thing is the integer_type_node, that looks wrong when you are

> dividing ssizetype by ssizetype.  The fold-const.c folders are

> sensitive to

> using incorrect types and type mismatches.

> And, I think fold_build2 is wasteful, you only care if you can

> simplify it

> to a constant (just constant or INTEGER_CST only?).

> So, either use fold_binary (TRUNC_DIV_EXPR, ssizetype, offset_cst,

> 			    fold_convert (ssizetype, byte_size))

> or, if you have checked that both arguments are INTEGER_CSTs, perhaps

> int_const_binop or so.

> 

> >        if (CONSTANT_CLASS_P (index))

> 

> And this would need to be if (index && TREE_CODE (index) ==

> INTEGER_CST)

> (or if you can handle other constants (index && CONSTANT_CLASS_P

> (index)).

> 

> >  	return get_or_create_constant_svalue (index);

> 

> 	Jakub


Thanks.

Here's an updated version of the patch which fold_converts both
inputs to the division, and uses fold_binary rather than fold_build2.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;
verified with -m32 and -m64.

OK for master?

gcc/analyzer/ChangeLog:
	PR analyzer/93281
	* region-model.cc
	(region_model::convert_byte_offset_to_array_index): Convert to
	ssizetype before dividing by byte_size.  Use fold_binary rather
	than fold_build2 to avoid needlessly constructing a tree for the
	non-const case.
---
 gcc/analyzer/region-model.cc | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.21.0

Comments

Richard Biener Jan. 17, 2020, 7:03 a.m. | #1
On Thu, 16 Jan 2020, David Malcolm wrote:

> On Thu, 2020-01-16 at 09:14 +0100, Jakub Jelinek wrote:

> > On Wed, Jan 15, 2020 at 06:56:48PM -0500, David Malcolm wrote:

> > > gcc/analyzer/ChangeLog:

> > > 	PR analyzer/93281

> > > 	* region-model.cc

> > > 	(region_model::convert_byte_offset_to_array_index): Convert

> > > 	offset_cst to ssizetype before dividing by byte_size.

> > > ---

> > >  gcc/analyzer/region-model.cc | 5 ++++-

> > >  1 file changed, 4 insertions(+), 1 deletion(-)

> > > 

> > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-

> > > model.cc

> > > index 5c39be4fd7f..b62ddf82a40 100644

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

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

> > > @@ -6414,9 +6414,12 @@

> > > region_model::convert_byte_offset_to_array_index (tree ptr_type,

> > >        /* This might not be a constant.  */

> > >        tree byte_size = size_in_bytes (elem_type);

> > >  

> > > +      /* Ensure we're in a signed representation before doing the

> > > division.  */

> > > +      tree signed_offset_cst = fold_convert (ssizetype,

> > > offset_cst);

> > > +

> > >        tree index

> > >  	= fold_build2 (TRUNC_DIV_EXPR, integer_type_node,

> > > -		       offset_cst, byte_size);

> > > +		       signed_offset_cst, byte_size);

> > 

> > I'd say you want to fold_convert byte_size to ssizetype too.

> > Another thing is the integer_type_node, that looks wrong when you are

> > dividing ssizetype by ssizetype.  The fold-const.c folders are

> > sensitive to

> > using incorrect types and type mismatches.

> > And, I think fold_build2 is wasteful, you only care if you can

> > simplify it

> > to a constant (just constant or INTEGER_CST only?).

> > So, either use fold_binary (TRUNC_DIV_EXPR, ssizetype, offset_cst,

> > 			    fold_convert (ssizetype, byte_size))

> > or, if you have checked that both arguments are INTEGER_CSTs, perhaps

> > int_const_binop or so.

> > 

> > >        if (CONSTANT_CLASS_P (index))

> > 

> > And this would need to be if (index && TREE_CODE (index) ==

> > INTEGER_CST)

> > (or if you can handle other constants (index && CONSTANT_CLASS_P

> > (index)).

> > 

> > >  	return get_or_create_constant_svalue (index);

> > 

> > 	Jakub

> 

> Thanks.

> 

> Here's an updated version of the patch which fold_converts both

> inputs to the division, and uses fold_binary rather than fold_build2.

> 

> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;

> verified with -m32 and -m64.

> 

> OK for master?


OK.

> gcc/analyzer/ChangeLog:

> 	PR analyzer/93281

> 	* region-model.cc

> 	(region_model::convert_byte_offset_to_array_index): Convert to

> 	ssizetype before dividing by byte_size.  Use fold_binary rather

> 	than fold_build2 to avoid needlessly constructing a tree for the

> 	non-const case.

> ---

>  gcc/analyzer/region-model.cc | 10 ++++++----

>  1 file changed, 6 insertions(+), 4 deletions(-)

> 

> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc

> index 5c39be4fd7f..f67572e2d45 100644

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

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

> @@ -6414,11 +6414,13 @@ region_model::convert_byte_offset_to_array_index (tree ptr_type,

>        /* This might not be a constant.  */

>        tree byte_size = size_in_bytes (elem_type);

>  

> +      /* Try to get a constant by dividing, ensuring that we're in a

> +	 signed representation first.  */

>        tree index

> -	= fold_build2 (TRUNC_DIV_EXPR, integer_type_node,

> -		       offset_cst, byte_size);

> -

> -      if (CONSTANT_CLASS_P (index))

> +	= fold_binary (TRUNC_DIV_EXPR, ssizetype,

> +		       fold_convert (ssizetype, offset_cst),

> +		       fold_convert (ssizetype, byte_size));

> +      if (index && TREE_CODE (index) == INTEGER_CST)

>  	return get_or_create_constant_svalue (index);

>      }

>  

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Patch

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 5c39be4fd7f..f67572e2d45 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -6414,11 +6414,13 @@  region_model::convert_byte_offset_to_array_index (tree ptr_type,
       /* This might not be a constant.  */
       tree byte_size = size_in_bytes (elem_type);
 
+      /* Try to get a constant by dividing, ensuring that we're in a
+	 signed representation first.  */
       tree index
-	= fold_build2 (TRUNC_DIV_EXPR, integer_type_node,
-		       offset_cst, byte_size);
-
-      if (CONSTANT_CLASS_P (index))
+	= fold_binary (TRUNC_DIV_EXPR, ssizetype,
+		       fold_convert (ssizetype, offset_cst),
+		       fold_convert (ssizetype, byte_size));
+      if (index && TREE_CODE (index) == INTEGER_CST)
 	return get_or_create_constant_svalue (index);
     }