diff mbox series

Add UBSAN_{PTR,BOUNDS} folding (PR sanitizer/81981)

Message ID 20170901111654.GE2323@tucnak
State New
Headers show
Series Add UBSAN_{PTR,BOUNDS} folding (PR sanitizer/81981) | expand

Commit Message

Jakub Jelinek Sept. 1, 2017, 11:16 a.m. UTC 
Hi!

This patch fixes the following testcase by folding some ubsan internal fns
we'd either remove anyway during sanopt, or lower into if (cond)
do_something during sanopt where cond would be always false.

Additionally, I've tried to clean up a little bit IFN_UBSAN_OBJECT_SIZE
handling by using variables for the call arguments that make it clear
what the arguments are.

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

2017-09-01  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/81981
	* gimple-fold.c (gimple_fold_call): Optimize away useless UBSAN_PTR
	and UBSAN_BOUNDS internal calls.  Clean up IFN_UBSAN_OBJECT_SIZE
	handling.

	* gcc.dg/ubsan/pr81981.c: New test.


	Jakub

Comments

Richard Biener Sept. 1, 2017, 12:32 p.m. UTC | #1 
On September 1, 2017 1:16:54 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>This patch fixes the following testcase by folding some ubsan internal
>fns
>we'd either remove anyway during sanopt, or lower into if (cond)
>do_something during sanopt where cond would be always false.
>
>Additionally, I've tried to clean up a little bit IFN_UBSAN_OBJECT_SIZE
>handling by using variables for the call arguments that make it clear
>what the arguments are.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I think there's a helper for replace - with-nop. 

Richard. 

>2017-09-01  Jakub Jelinek  <jakub@redhat.com>
>
>	PR sanitizer/81981
>	* gimple-fold.c (gimple_fold_call): Optimize away useless UBSAN_PTR
>	and UBSAN_BOUNDS internal calls.  Clean up IFN_UBSAN_OBJECT_SIZE
>	handling.
>
>	* gcc.dg/ubsan/pr81981.c: New test.
>
>--- gcc/gimple-fold.c.jj	2017-08-10 02:31:21.000000000 +0200
>+++ gcc/gimple-fold.c	2017-08-29 18:50:49.993673432 +0200
>@@ -3938,11 +3938,23 @@ gimple_fold_call (gimple_stmt_iterator *
> 					gimple_call_arg (stmt, 2));
> 	  break;
> 	case IFN_UBSAN_OBJECT_SIZE:
>-	  if (integer_all_onesp (gimple_call_arg (stmt, 2))
>-	      || (TREE_CODE (gimple_call_arg (stmt, 1)) == INTEGER_CST
>-		  && TREE_CODE (gimple_call_arg (stmt, 2)) == INTEGER_CST
>-		  && tree_int_cst_le (gimple_call_arg (stmt, 1),
>-				      gimple_call_arg (stmt, 2))))
>+	  {
>+	    tree offset = gimple_call_arg (stmt, 1);
>+	    tree objsize = gimple_call_arg (stmt, 2);
>+	    if (integer_all_onesp (objsize)
>+		|| (TREE_CODE (offset) == INTEGER_CST
>+		    && TREE_CODE (objsize) == INTEGER_CST
>+		    && tree_int_cst_le (offset, objsize)))
>+	      {
>+		gsi_replace (gsi, gimple_build_nop (), false);
>+		unlink_stmt_vdef (stmt);
>+		release_defs (stmt);
>+		return true;
>+	      }
>+	  }
>+	  break;
>+	case IFN_UBSAN_PTR:
>+	  if (integer_zerop (gimple_call_arg (stmt, 1)))
> 	    {
> 	      gsi_replace (gsi, gimple_build_nop (), false);
> 	      unlink_stmt_vdef (stmt);
>@@ -3950,6 +3962,25 @@ gimple_fold_call (gimple_stmt_iterator *
> 	      return true;
> 	    }
> 	  break;
>+	case IFN_UBSAN_BOUNDS:
>+	  {
>+	    tree index = gimple_call_arg (stmt, 1);
>+	    tree bound = gimple_call_arg (stmt, 2);
>+	    if (TREE_CODE (index) == INTEGER_CST
>+		&& TREE_CODE (bound) == INTEGER_CST)
>+	      {
>+		index = fold_convert (TREE_TYPE (bound), index);
>+		if (TREE_CODE (index) == INTEGER_CST
>+		    && tree_int_cst_le (index, bound))
>+		  {
>+		    gsi_replace (gsi, gimple_build_nop (), false);
>+		    unlink_stmt_vdef (stmt);
>+		    release_defs (stmt);
>+		    return true;
>+		  }
>+	      }
>+	  }
>+	  break;
> 	case IFN_GOACC_DIM_SIZE:
> 	case IFN_GOACC_DIM_POS:
> 	  result = fold_internal_goacc_dim (stmt);
>--- gcc/testsuite/gcc.dg/ubsan/pr81981.c.jj	2017-08-29
>18:54:33.826107761 +0200
>+++ gcc/testsuite/gcc.dg/ubsan/pr81981.c	2017-08-29 18:55:36.721386827
>+0200
>@@ -0,0 +1,21 @@
>+/* PR sanitizer/81981 */
>+/* { dg-do compile } */
>+/* { dg-options "-O2 -Wmaybe-uninitialized -fsanitize=undefined
>-ffat-lto-objects" } */
>+
>+int v;
>+
>+int
>+foo (int i)
>+{
>+  int t[1], u[1];
>+  int n = 0;
>+
>+  if (i)
>+    {
>+      t[n] = i;
>+      u[0] = i;
>+    }
>+
>+  v = u[0];		/* { dg-warning "may be used uninitialized in this
>function" } */
>+  return t[0];		/* { dg-warning "may be used uninitialized in this
>function" } */
>+}
>
>	Jakub
Jakub Jelinek Sept. 1, 2017, 1:53 p.m. UTC | #2 
On Fri, Sep 01, 2017 at 02:32:43PM +0200, Richard Biener wrote:
> On September 1, 2017 1:16:54 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
> >Hi!
> >
> >This patch fixes the following testcase by folding some ubsan internal
> >fns
> >we'd either remove anyway during sanopt, or lower into if (cond)
> >do_something during sanopt where cond would be always false.
> >
> >Additionally, I've tried to clean up a little bit IFN_UBSAN_OBJECT_SIZE
> >handling by using variables for the call arguments that make it clear
> >what the arguments are.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> I think there's a helper for replace - with-nop. 

Can't find it.
gimplify_and_update_call_from_tree has to add it, but I'd need
to call it with something that gimplifies into empty sequence (void_node?)
and it would still go through push_gimplify_context/gimplify_and_add/pop_gimplify_context
etc., which looks quite expensive.

> >2017-09-01  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR sanitizer/81981
> >	* gimple-fold.c (gimple_fold_call): Optimize away useless UBSAN_PTR
> >	and UBSAN_BOUNDS internal calls.  Clean up IFN_UBSAN_OBJECT_SIZE
> >	handling.
> >
> >	* gcc.dg/ubsan/pr81981.c: New test.
> >
> >--- gcc/gimple-fold.c.jj	2017-08-10 02:31:21.000000000 +0200
> >+++ gcc/gimple-fold.c	2017-08-29 18:50:49.993673432 +0200
> >@@ -3938,11 +3938,23 @@ gimple_fold_call (gimple_stmt_iterator *
> > 					gimple_call_arg (stmt, 2));
> > 	  break;
> > 	case IFN_UBSAN_OBJECT_SIZE:
> >-	  if (integer_all_onesp (gimple_call_arg (stmt, 2))
> >-	      || (TREE_CODE (gimple_call_arg (stmt, 1)) == INTEGER_CST
> >-		  && TREE_CODE (gimple_call_arg (stmt, 2)) == INTEGER_CST
> >-		  && tree_int_cst_le (gimple_call_arg (stmt, 1),
> >-				      gimple_call_arg (stmt, 2))))
> >+	  {
> >+	    tree offset = gimple_call_arg (stmt, 1);
> >+	    tree objsize = gimple_call_arg (stmt, 2);
> >+	    if (integer_all_onesp (objsize)
> >+		|| (TREE_CODE (offset) == INTEGER_CST
> >+		    && TREE_CODE (objsize) == INTEGER_CST
> >+		    && tree_int_cst_le (offset, objsize)))
> >+	      {
> >+		gsi_replace (gsi, gimple_build_nop (), false);
> >+		unlink_stmt_vdef (stmt);
> >+		release_defs (stmt);
> >+		return true;
> >+	      }
> >+	  }
> >+	  break;
> >+	case IFN_UBSAN_PTR:
> >+	  if (integer_zerop (gimple_call_arg (stmt, 1)))
> > 	    {
> > 	      gsi_replace (gsi, gimple_build_nop (), false);
> > 	      unlink_stmt_vdef (stmt);
> >@@ -3950,6 +3962,25 @@ gimple_fold_call (gimple_stmt_iterator *
> > 	      return true;
> > 	    }
> > 	  break;
> >+	case IFN_UBSAN_BOUNDS:
> >+	  {
> >+	    tree index = gimple_call_arg (stmt, 1);
> >+	    tree bound = gimple_call_arg (stmt, 2);
> >+	    if (TREE_CODE (index) == INTEGER_CST
> >+		&& TREE_CODE (bound) == INTEGER_CST)
> >+	      {
> >+		index = fold_convert (TREE_TYPE (bound), index);
> >+		if (TREE_CODE (index) == INTEGER_CST
> >+		    && tree_int_cst_le (index, bound))
> >+		  {
> >+		    gsi_replace (gsi, gimple_build_nop (), false);
> >+		    unlink_stmt_vdef (stmt);
> >+		    release_defs (stmt);
> >+		    return true;
> >+		  }
> >+	      }
> >+	  }
> >+	  break;
> > 	case IFN_GOACC_DIM_SIZE:
> > 	case IFN_GOACC_DIM_POS:
> > 	  result = fold_internal_goacc_dim (stmt);
> >--- gcc/testsuite/gcc.dg/ubsan/pr81981.c.jj	2017-08-29
> >18:54:33.826107761 +0200
> >+++ gcc/testsuite/gcc.dg/ubsan/pr81981.c	2017-08-29 18:55:36.721386827
> >+0200
> >@@ -0,0 +1,21 @@
> >+/* PR sanitizer/81981 */
> >+/* { dg-do compile } */
> >+/* { dg-options "-O2 -Wmaybe-uninitialized -fsanitize=undefined
> >-ffat-lto-objects" } */
> >+
> >+int v;
> >+
> >+int
> >+foo (int i)
> >+{
> >+  int t[1], u[1];
> >+  int n = 0;
> >+
> >+  if (i)
> >+    {
> >+      t[n] = i;
> >+      u[0] = i;
> >+    }
> >+
> >+  v = u[0];		/* { dg-warning "may be used uninitialized in this
> >function" } */
> >+  return t[0];		/* { dg-warning "may be used uninitialized in this
> >function" } */
> >+}
> >
> >	Jakub

	Jakub
Richard Biener Sept. 1, 2017, 5:10 p.m. UTC | #3 
On September 1, 2017 3:53:28 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Fri, Sep 01, 2017 at 02:32:43PM +0200, Richard Biener wrote:
>> On September 1, 2017 1:16:54 PM GMT+02:00, Jakub Jelinek
><jakub@redhat.com> wrote:
>> >Hi!
>> >
>> >This patch fixes the following testcase by folding some ubsan
>internal
>> >fns
>> >we'd either remove anyway during sanopt, or lower into if (cond)
>> >do_something during sanopt where cond would be always false.
>> >
>> >Additionally, I've tried to clean up a little bit
>IFN_UBSAN_OBJECT_SIZE
>> >handling by using variables for the call arguments that make it
>clear
>> >what the arguments are.
>> >
>> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> 
>> I think there's a helper for replace - with-nop. 
>
>Can't find it.
>gimplify_and_update_call_from_tree has to add it, but I'd need
>to call it with something that gimplifies into empty sequence
>(void_node?)
>and it would still go through
>push_gimplify_context/gimplify_and_add/pop_gimplify_context
>etc., which looks quite expensive.

OK, I thought we have one. Can you add a helper for it please? replace_with_nop or so? I thought there's maybe replace_with_value which handles null lhs by replacing with nop. (can't check, writing from phone) 

Richard. 

>> >2017-09-01  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >	PR sanitizer/81981
>> >	* gimple-fold.c (gimple_fold_call): Optimize away useless UBSAN_PTR
>> >	and UBSAN_BOUNDS internal calls.  Clean up IFN_UBSAN_OBJECT_SIZE
>> >	handling.
>> >
>> >	* gcc.dg/ubsan/pr81981.c: New test.
>> >
>> >--- gcc/gimple-fold.c.jj	2017-08-10 02:31:21.000000000 +0200
>> >+++ gcc/gimple-fold.c	2017-08-29 18:50:49.993673432 +0200
>> >@@ -3938,11 +3938,23 @@ gimple_fold_call (gimple_stmt_iterator *
>> > 					gimple_call_arg (stmt, 2));
>> > 	  break;
>> > 	case IFN_UBSAN_OBJECT_SIZE:
>> >-	  if (integer_all_onesp (gimple_call_arg (stmt, 2))
>> >-	      || (TREE_CODE (gimple_call_arg (stmt, 1)) == INTEGER_CST
>> >-		  && TREE_CODE (gimple_call_arg (stmt, 2)) == INTEGER_CST
>> >-		  && tree_int_cst_le (gimple_call_arg (stmt, 1),
>> >-				      gimple_call_arg (stmt, 2))))
>> >+	  {
>> >+	    tree offset = gimple_call_arg (stmt, 1);
>> >+	    tree objsize = gimple_call_arg (stmt, 2);
>> >+	    if (integer_all_onesp (objsize)
>> >+		|| (TREE_CODE (offset) == INTEGER_CST
>> >+		    && TREE_CODE (objsize) == INTEGER_CST
>> >+		    && tree_int_cst_le (offset, objsize)))
>> >+	      {
>> >+		gsi_replace (gsi, gimple_build_nop (), false);
>> >+		unlink_stmt_vdef (stmt);
>> >+		release_defs (stmt);
>> >+		return true;
>> >+	      }
>> >+	  }
>> >+	  break;
>> >+	case IFN_UBSAN_PTR:
>> >+	  if (integer_zerop (gimple_call_arg (stmt, 1)))
>> > 	    {
>> > 	      gsi_replace (gsi, gimple_build_nop (), false);
>> > 	      unlink_stmt_vdef (stmt);
>> >@@ -3950,6 +3962,25 @@ gimple_fold_call (gimple_stmt_iterator *
>> > 	      return true;
>> > 	    }
>> > 	  break;
>> >+	case IFN_UBSAN_BOUNDS:
>> >+	  {
>> >+	    tree index = gimple_call_arg (stmt, 1);
>> >+	    tree bound = gimple_call_arg (stmt, 2);
>> >+	    if (TREE_CODE (index) == INTEGER_CST
>> >+		&& TREE_CODE (bound) == INTEGER_CST)
>> >+	      {
>> >+		index = fold_convert (TREE_TYPE (bound), index);
>> >+		if (TREE_CODE (index) == INTEGER_CST
>> >+		    && tree_int_cst_le (index, bound))
>> >+		  {
>> >+		    gsi_replace (gsi, gimple_build_nop (), false);
>> >+		    unlink_stmt_vdef (stmt);
>> >+		    release_defs (stmt);
>> >+		    return true;
>> >+		  }
>> >+	      }
>> >+	  }
>> >+	  break;
>> > 	case IFN_GOACC_DIM_SIZE:
>> > 	case IFN_GOACC_DIM_POS:
>> > 	  result = fold_internal_goacc_dim (stmt);
>> >--- gcc/testsuite/gcc.dg/ubsan/pr81981.c.jj	2017-08-29
>> >18:54:33.826107761 +0200
>> >+++ gcc/testsuite/gcc.dg/ubsan/pr81981.c	2017-08-29
>18:55:36.721386827
>> >+0200
>> >@@ -0,0 +1,21 @@
>> >+/* PR sanitizer/81981 */
>> >+/* { dg-do compile } */
>> >+/* { dg-options "-O2 -Wmaybe-uninitialized -fsanitize=undefined
>> >-ffat-lto-objects" } */
>> >+
>> >+int v;
>> >+
>> >+int
>> >+foo (int i)
>> >+{
>> >+  int t[1], u[1];
>> >+  int n = 0;
>> >+
>> >+  if (i)
>> >+    {
>> >+      t[n] = i;
>> >+      u[0] = i;
>> >+    }
>> >+
>> >+  v = u[0];		/* { dg-warning "may be used uninitialized in this
>> >function" } */
>> >+  return t[0];		/* { dg-warning "may be used uninitialized in this
>> >function" } */
>> >+}
>> >
>> >	Jakub
>
>	Jakub
diff mbox series

Patch

--- gcc/gimple-fold.c.jj	2017-08-10 02:31:21.000000000 +0200
+++ gcc/gimple-fold.c	2017-08-29 18:50:49.993673432 +0200
@@ -3938,11 +3938,23 @@  gimple_fold_call (gimple_stmt_iterator *
 					gimple_call_arg (stmt, 2));
 	  break;
 	case IFN_UBSAN_OBJECT_SIZE:
-	  if (integer_all_onesp (gimple_call_arg (stmt, 2))
-	      || (TREE_CODE (gimple_call_arg (stmt, 1)) == INTEGER_CST
-		  && TREE_CODE (gimple_call_arg (stmt, 2)) == INTEGER_CST
-		  && tree_int_cst_le (gimple_call_arg (stmt, 1),
-				      gimple_call_arg (stmt, 2))))
+	  {
+	    tree offset = gimple_call_arg (stmt, 1);
+	    tree objsize = gimple_call_arg (stmt, 2);
+	    if (integer_all_onesp (objsize)
+		|| (TREE_CODE (offset) == INTEGER_CST
+		    && TREE_CODE (objsize) == INTEGER_CST
+		    && tree_int_cst_le (offset, objsize)))
+	      {
+		gsi_replace (gsi, gimple_build_nop (), false);
+		unlink_stmt_vdef (stmt);
+		release_defs (stmt);
+		return true;
+	      }
+	  }
+	  break;
+	case IFN_UBSAN_PTR:
+	  if (integer_zerop (gimple_call_arg (stmt, 1)))
 	    {
 	      gsi_replace (gsi, gimple_build_nop (), false);
 	      unlink_stmt_vdef (stmt);
@@ -3950,6 +3962,25 @@  gimple_fold_call (gimple_stmt_iterator *
 	      return true;
 	    }
 	  break;
+	case IFN_UBSAN_BOUNDS:
+	  {
+	    tree index = gimple_call_arg (stmt, 1);
+	    tree bound = gimple_call_arg (stmt, 2);
+	    if (TREE_CODE (index) == INTEGER_CST
+		&& TREE_CODE (bound) == INTEGER_CST)
+	      {
+		index = fold_convert (TREE_TYPE (bound), index);
+		if (TREE_CODE (index) == INTEGER_CST
+		    && tree_int_cst_le (index, bound))
+		  {
+		    gsi_replace (gsi, gimple_build_nop (), false);
+		    unlink_stmt_vdef (stmt);
+		    release_defs (stmt);
+		    return true;
+		  }
+	      }
+	  }
+	  break;
 	case IFN_GOACC_DIM_SIZE:
 	case IFN_GOACC_DIM_POS:
 	  result = fold_internal_goacc_dim (stmt);
--- gcc/testsuite/gcc.dg/ubsan/pr81981.c.jj	2017-08-29 18:54:33.826107761 +0200
+++ gcc/testsuite/gcc.dg/ubsan/pr81981.c	2017-08-29 18:55:36.721386827 +0200
@@ -0,0 +1,21 @@ 
+/* PR sanitizer/81981 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wmaybe-uninitialized -fsanitize=undefined -ffat-lto-objects" } */
+
+int v;
+
+int
+foo (int i)
+{
+  int t[1], u[1];
+  int n = 0;
+
+  if (i)
+    {
+      t[n] = i;
+      u[0] = i;
+    }
+
+  v = u[0];		/* { dg-warning "may be used uninitialized in this function" } */
+  return t[0];		/* { dg-warning "may be used uninitialized in this function" } */
+}