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

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

Commit Message

David Malcolm Jan. 15, 2020, 11:56 p.m.
Various 32-bit targets show failures in gcc.dg/analyzer/data-model-1.c
with tests of the form:
  __analyzer_eval (q[-2].x == 107024); /* { dg-warning "TRUE" } */
  __analyzer_eval (q[-2].y == 107025); /* { dg-warning "TRUE" } */
where they emit UNKNOWN instead.

The root cause is that gimple has a byte-based twos-complement offset
of -16 expressed like this:
  _55 = q_92 + 4294967280;  (32-bit)
or:
  _55 = q_92 + 18446744073709551600; (64-bit)

Within region_model::convert_byte_offset_to_array_index that unsigned
offset was being divided by the element size to get an offset within
an array.

This happened to work on 64-bit target and host, but not elsewhere;
the offset needs to be converted to a signed type before the division
is meaningful.

This patch does so, fixing the failures.

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
	offset_cst to ssizetype before dividing by byte_size.
---
 gcc/analyzer/region-model.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.21.0

Comments

Richard Biener Jan. 16, 2020, 7:07 a.m. | #1
On January 16, 2020 12:56:48 AM GMT+01:00, David Malcolm <dmalcolm@redhat.com> wrote:
>Various 32-bit targets show failures in gcc.dg/analyzer/data-model-1.c

>with tests of the form:

>  __analyzer_eval (q[-2].x == 107024); /* { dg-warning "TRUE" } */

>  __analyzer_eval (q[-2].y == 107025); /* { dg-warning "TRUE" } */

>where they emit UNKNOWN instead.

>

>The root cause is that gimple has a byte-based twos-complement offset

>of -16 expressed like this:

>  _55 = q_92 + 4294967280;  (32-bit)

>or:

>  _55 = q_92 + 18446744073709551600; (64-bit)

>

>Within region_model::convert_byte_offset_to_array_index that unsigned

>offset was being divided by the element size to get an offset within

>an array.

>

>This happened to work on 64-bit target and host, but not elsewhere;

>the offset needs to be converted to a signed type before the division

>is meaningful.

>

>This patch does so, fixing the failures.

>

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

>verified with -m32 and -m64.

>

>OK for master?


Technically it depends on where you get that offset from. For MEM_REF there's mem_ref_offset which does the change for you for example. 

But, OK. 

Richard. 

>

>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);

> 

>       if (CONSTANT_CLASS_P (index))

> 	return get_or_create_constant_svalue (index);
Jakub Jelinek Jan. 16, 2020, 8:14 a.m. | #2
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

Patch

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);
 
       if (CONSTANT_CLASS_P (index))
 	return get_or_create_constant_svalue (index);