diff mbox series

Fix ptr-overflow sanopt optimization (PR sanitizer/83392)

Message ID 20180313202240.GD8577@tucnak
State New
Headers show
Series Fix ptr-overflow sanopt optimization (PR sanitizer/83392) | expand

Commit Message

Jakub Jelinek March 13, 2018, 8:22 p.m. UTC
Hi!

The sanopt maybe_optimize_ubsan_ptr_ifn optimization behaves differently
on 32-bit and on 64-bit targets when using similar arguments maximum or
minimum of ptrdiff_t or values close to it.

The problem is that UHWI is 64-bit, regardless of whether addresses are
64-bit or 32-bit, so for 32-bit targets get_inner_reference returns
NULL offset and all the offset is in pbitpos, while on 64-bit targets
where such large offsets in bytes would fit into shwi, but in bits won't
fit, we return INTEGER_CST offset and often 0 pbitpos.

The following patch handles such offset, so that the sanopt behaves the
same way, and adjusts the testcase (which really should have just 14
matches, the 3 lines with comment changes are already covered by the
overflow check on p = b - SMAX;).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-03-13  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/83392
	* sanopt.c (maybe_optimize_ubsan_ptr_ifn): Handle also
	INTEGER_CST offset, add it together with bitpos / 8 and
	sign extend based on POINTER_SIZE.

	* c-c++-common/ubsan/ptr-overflow-sanitization-1.c: Adjust expected
	check count from 17 to 14.


	Jakub

Comments

Richard Biener March 14, 2018, 8:44 a.m. UTC | #1
On Tue, 13 Mar 2018, Jakub Jelinek wrote:

> Hi!
> 
> The sanopt maybe_optimize_ubsan_ptr_ifn optimization behaves differently
> on 32-bit and on 64-bit targets when using similar arguments maximum or
> minimum of ptrdiff_t or values close to it.
> 
> The problem is that UHWI is 64-bit, regardless of whether addresses are
> 64-bit or 32-bit, so for 32-bit targets get_inner_reference returns
> NULL offset and all the offset is in pbitpos, while on 64-bit targets
> where such large offsets in bytes would fit into shwi, but in bits won't
> fit, we return INTEGER_CST offset and often 0 pbitpos.
> 
> The following patch handles such offset, so that the sanopt behaves the
> same way, and adjusts the testcase (which really should have just 14
> matches, the 3 lines with comment changes are already covered by the
> overflow check on p = b - SMAX;).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2018-03-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/83392
> 	* sanopt.c (maybe_optimize_ubsan_ptr_ifn): Handle also
> 	INTEGER_CST offset, add it together with bitpos / 8 and
> 	sign extend based on POINTER_SIZE.
> 
> 	* c-c++-common/ubsan/ptr-overflow-sanitization-1.c: Adjust expected
> 	check count from 17 to 14.
> 
> --- gcc/sanopt.c.jj	2018-03-02 00:15:54.670780980 +0100
> +++ gcc/sanopt.c	2018-03-13 16:54:49.905621373 +0100
> @@ -486,12 +486,17 @@ maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
>        HOST_WIDE_INT bitpos;
>        base = get_inner_reference (base, &bitsize, &pbitpos, &offset, &mode,
>  				  &unsignedp, &reversep, &volatilep);
> -      if (offset == NULL_TREE
> +      if ((offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
>  	  && DECL_P (base)
>  	  && pbitpos.is_constant (&bitpos))
>  	{
>  	  gcc_assert (!DECL_REGISTER (base));
> -	  offset_int expr_offset = bitpos / BITS_PER_UNIT;
> +	  offset_int expr_offset;
> +	  if (offset)
> +	    expr_offset = wi::to_offset (offset) + bitpos / BITS_PER_UNIT;
> +	  else
> +	    expr_offset = bitpos / BITS_PER_UNIT;
> +	  expr_offset = wi::sext (expr_offset, POINTER_SIZE);
>  	  offset_int total_offset = expr_offset + cur_offset;
>  	  if (total_offset != wi::sext (total_offset, POINTER_SIZE))
>  	    {
> @@ -511,7 +516,7 @@ maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
>  	      && (!is_global_var (base) || decl_binds_to_current_def_p (base)))
>  	    {
>  	      offset_int base_size = wi::to_offset (DECL_SIZE_UNIT (base));
> -	      if (bitpos >= 0
> +	      if (!wi::neg_p (expr_offset)
>  		  && wi::les_p (total_offset, base_size))
>  		{
>  		  if (!wi::neg_p (total_offset)
> @@ -532,7 +537,7 @@ maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
>  	     */
>  
>  	  bool sign_cur_offset = !wi::neg_p (cur_offset);
> -	  bool sign_expr_offset = bitpos >= 0;
> +	  bool sign_expr_offset = !wi::neg_p (expr_offset);
>  
>  	  tree base_addr
>  	    = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (base)), base);
> --- gcc/testsuite/c-c++-common/ubsan/ptr-overflow-sanitization-1.c.jj	2017-10-11 22:37:52.798901780 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/ptr-overflow-sanitization-1.c	2018-03-13 17:00:18.947808006 +0100
> @@ -25,9 +25,9 @@ void foo(void)
>    p2 = p + 2;
>  
>    p = b - SMAX; /* pointer overflow check is needed */
> -  p2 = p + (SMAX - 2); /* b - 2: pointer overflow check is needed */
> -  p2 = p + (SMAX - 1); /* b - 1: pointer overflow check is needed */
> -  p2 = p + SMAX; /* b: pointer overflow check is needed */
> +  p2 = p + (SMAX - 2); /* b - 2: no need to check this  */
> +  p2 = p + (SMAX - 1); /* b - 1: no need to check this */
> +  p2 = p + SMAX; /* b: no need to check this */
>    p2++; /* b + 1 */
>  
>    p = c;
> @@ -75,4 +75,4 @@ void negative_to_negative (char *ptr)
>    p2 += 5;
>  }
>  
> -/* { dg-final { scan-tree-dump-times "__ubsan_handle_pointer_overflow" 17 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times "__ubsan_handle_pointer_overflow" 14 "optimized" } } */
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- gcc/sanopt.c.jj	2018-03-02 00:15:54.670780980 +0100
+++ gcc/sanopt.c	2018-03-13 16:54:49.905621373 +0100
@@ -486,12 +486,17 @@  maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
       HOST_WIDE_INT bitpos;
       base = get_inner_reference (base, &bitsize, &pbitpos, &offset, &mode,
 				  &unsignedp, &reversep, &volatilep);
-      if (offset == NULL_TREE
+      if ((offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
 	  && DECL_P (base)
 	  && pbitpos.is_constant (&bitpos))
 	{
 	  gcc_assert (!DECL_REGISTER (base));
-	  offset_int expr_offset = bitpos / BITS_PER_UNIT;
+	  offset_int expr_offset;
+	  if (offset)
+	    expr_offset = wi::to_offset (offset) + bitpos / BITS_PER_UNIT;
+	  else
+	    expr_offset = bitpos / BITS_PER_UNIT;
+	  expr_offset = wi::sext (expr_offset, POINTER_SIZE);
 	  offset_int total_offset = expr_offset + cur_offset;
 	  if (total_offset != wi::sext (total_offset, POINTER_SIZE))
 	    {
@@ -511,7 +516,7 @@  maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
 	      && (!is_global_var (base) || decl_binds_to_current_def_p (base)))
 	    {
 	      offset_int base_size = wi::to_offset (DECL_SIZE_UNIT (base));
-	      if (bitpos >= 0
+	      if (!wi::neg_p (expr_offset)
 		  && wi::les_p (total_offset, base_size))
 		{
 		  if (!wi::neg_p (total_offset)
@@ -532,7 +537,7 @@  maybe_optimize_ubsan_ptr_ifn (sanopt_ctx
 	     */
 
 	  bool sign_cur_offset = !wi::neg_p (cur_offset);
-	  bool sign_expr_offset = bitpos >= 0;
+	  bool sign_expr_offset = !wi::neg_p (expr_offset);
 
 	  tree base_addr
 	    = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (base)), base);
--- gcc/testsuite/c-c++-common/ubsan/ptr-overflow-sanitization-1.c.jj	2017-10-11 22:37:52.798901780 +0200
+++ gcc/testsuite/c-c++-common/ubsan/ptr-overflow-sanitization-1.c	2018-03-13 17:00:18.947808006 +0100
@@ -25,9 +25,9 @@  void foo(void)
   p2 = p + 2;
 
   p = b - SMAX; /* pointer overflow check is needed */
-  p2 = p + (SMAX - 2); /* b - 2: pointer overflow check is needed */
-  p2 = p + (SMAX - 1); /* b - 1: pointer overflow check is needed */
-  p2 = p + SMAX; /* b: pointer overflow check is needed */
+  p2 = p + (SMAX - 2); /* b - 2: no need to check this  */
+  p2 = p + (SMAX - 1); /* b - 1: no need to check this */
+  p2 = p + SMAX; /* b: no need to check this */
   p2++; /* b + 1 */
 
   p = c;
@@ -75,4 +75,4 @@  void negative_to_negative (char *ptr)
   p2 += 5;
 }
 
-/* { dg-final { scan-tree-dump-times "__ubsan_handle_pointer_overflow" 17 "optimized" } } */
+/* { dg-final { scan-tree-dump-times "__ubsan_handle_pointer_overflow" 14 "optimized" } } */