diff mbox

Limit removal of UBSAN_NULL checks

Message ID 20141105213745.GZ20462@redhat.com
State New
Headers show

Commit Message

Marek Polacek Nov. 5, 2014, 9:37 p.m. UTC 
This patch limits optimizing UBSAN_NULL checks a bit, so that we
don't lose diagnostics.
If we know we're on a path where segv would have already occured,
we can drop further checks (e.g. dereferencing a NULL pointer - we
can't recover from that).  Also if we're not recovering or using
-fsanitize-undefined-trap-on-error we can clear checks that are
dominated.  Furthermore, if we have more checks for the same location,
there's no point in keeping all of them.

Bootstrap-ubsan/regtest passed on x86_64-linux, ok for trunk?

2014-11-05  Marek Polacek  <polacek@redhat.com>

	* sanopt.c (sanopt_optimize_walker): Limit removal of the checks.
testsuite/
	* c-c++-common/ubsan/align-2.c: Add dg-output.
	* c-c++-common/ubsan/align-4.c: Likewise.
	* c-c++-common/ubsan/align-6.c: New test.
	* c-c++-common/ubsan/align-7.c: New test.
	* c-c++-common/ubsan/align-8.c: New test.
	* g++.dg/ubsan/null-1.C: Add dg-output.
	* g++.dg/ubsan/null-2.C: Likewise.
	* g++.dg/ubsan/null-3.C: New test.
	* g++.dg/ubsan/null-4.C: New test.
	* g++.dg/ubsan/null-5.C: New test.


	Marek

Comments

Jakub Jelinek Nov. 5, 2014, 10:08 p.m. UTC | #1 
On Wed, Nov 05, 2014 at 10:37:46PM +0100, Marek Polacek wrote:
> 2014-11-05  Marek Polacek  <polacek@redhat.com>
> 
> 	* sanopt.c (sanopt_optimize_walker): Limit removal of the checks.
> testsuite/
> 	* c-c++-common/ubsan/align-2.c: Add dg-output.
> 	* c-c++-common/ubsan/align-4.c: Likewise.
> 	* c-c++-common/ubsan/align-6.c: New test.
> 	* c-c++-common/ubsan/align-7.c: New test.
> 	* c-c++-common/ubsan/align-8.c: New test.
> 	* g++.dg/ubsan/null-1.C: Add dg-output.
> 	* g++.dg/ubsan/null-2.C: Likewise.
> 	* g++.dg/ubsan/null-3.C: New test.
> 	* g++.dg/ubsan/null-4.C: New test.
> 	* g++.dg/ubsan/null-5.C: New test.

Looks good, though:

> diff --git gcc/sanopt.c gcc/sanopt.c
> index 4483ff7..cb172d3 100644
> --- gcc/sanopt.c
> +++ gcc/sanopt.c
> @@ -130,7 +130,27 @@ sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
>  			  /* At this point we shouldn't have any statements
>  			     that aren't dominating the current BB.  */
>  			  tree align = gimple_call_arg (g, 2);
> -			  remove = tree_int_cst_le (cur_align, align);
> +			  int kind = tree_to_shwi (gimple_call_arg (g, 1));
> +			  /* If this is a NULL pointer check where we had segv
> +			     anyway, we can remove it.  */
> +			  if (integer_zerop (align)
> +			      && (kind == UBSAN_LOAD_OF
> +				  || kind == UBSAN_STORE_OF
> +				  || kind == UBSAN_MEMBER_ACCESS))
> +			    remove = true;
> +			  /* Otherwise remove the check in non-recovering
> +			     mode, or if the stmts have same location.  */
> +			  else if (integer_zerop (align))
> +			    remove = !(flag_sanitize_recover & SANITIZE_NULL)
> +				     || flag_sanitize_undefined_trap_on_error
> +				     || gimple_location (g)
> +					== gimple_location (stmt);
> +			  else if (tree_int_cst_le (cur_align, align))
> +			    remove = !(flag_sanitize_recover
> +				       & SANITIZE_ALIGNMENT)
> +				     || flag_sanitize_undefined_trap_on_error
> +				     || gimple_location (g)
> +					== gimple_location (stmt);

I wonder if for the case where you don't remove, gimple_bb (g) == gimple_bb
(stmt) and tree_int_cst_compare (cur_align, align) == 0 it wouldn't be
worthwhile to v.pop (); so that the vector doesn't grow up unnecessarily too
much.  Maybe also bump the 30 limit to 100-200?  Now that you don't walk the
whole vector each time, it is only a matter of memory consumption (perhaps
instead of having a constant limit for every vector it might be better to
just have a limit on the total size of all the sanopt vectors (say a few
megabytes)?  And count towards that limit the allocated () size of the
vector, once you have some size allocated, not pushing stuff in there
doesn't make you consume more memory.  Or just don't have a limit at all.

	Jakub
diff mbox

Patch

diff --git gcc/sanopt.c gcc/sanopt.c
index 4483ff7..cb172d3 100644
--- gcc/sanopt.c
+++ gcc/sanopt.c
@@ -130,7 +130,27 @@  sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
 			  /* At this point we shouldn't have any statements
 			     that aren't dominating the current BB.  */
 			  tree align = gimple_call_arg (g, 2);
-			  remove = tree_int_cst_le (cur_align, align);
+			  int kind = tree_to_shwi (gimple_call_arg (g, 1));
+			  /* If this is a NULL pointer check where we had segv
+			     anyway, we can remove it.  */
+			  if (integer_zerop (align)
+			      && (kind == UBSAN_LOAD_OF
+				  || kind == UBSAN_STORE_OF
+				  || kind == UBSAN_MEMBER_ACCESS))
+			    remove = true;
+			  /* Otherwise remove the check in non-recovering
+			     mode, or if the stmts have same location.  */
+			  else if (integer_zerop (align))
+			    remove = !(flag_sanitize_recover & SANITIZE_NULL)
+				     || flag_sanitize_undefined_trap_on_error
+				     || gimple_location (g)
+					== gimple_location (stmt);
+			  else if (tree_int_cst_le (cur_align, align))
+			    remove = !(flag_sanitize_recover
+				       & SANITIZE_ALIGNMENT)
+				     || flag_sanitize_undefined_trap_on_error
+				     || gimple_location (g)
+					== gimple_location (stmt);
 			  break;
 			}
 		    }
diff --git gcc/testsuite/c-c++-common/ubsan/align-2.c gcc/testsuite/c-c++-common/ubsan/align-2.c
index 02a26e2..071de8c 100644
--- gcc/testsuite/c-c++-common/ubsan/align-2.c
+++ gcc/testsuite/c-c++-common/ubsan/align-2.c
@@ -51,4 +51,6 @@  main ()
 /* { dg-output "\.c:(13|16):\[0-9]*: \[^\n\r]*store to misaligned address 0x\[0-9a-fA-F]* for type 'int', which requires 4 byte alignment.*" } */
 /* { dg-output "\.c:23:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
 /* { dg-output "\.c:(29|30):\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:30:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:31:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
 /* { dg-output "\.c:37:\[0-9]*: \[^\n\r]*load of misaligned address 0x\[0-9a-fA-F]* for type 'long long int', which requires \[48] byte alignment" } */
diff --git gcc/testsuite/c-c++-common/ubsan/align-4.c gcc/testsuite/c-c++-common/ubsan/align-4.c
index f010589..3252595 100644
--- gcc/testsuite/c-c++-common/ubsan/align-4.c
+++ gcc/testsuite/c-c++-common/ubsan/align-4.c
@@ -9,4 +9,6 @@ 
 /* { dg-output "\[^\n\r]*\.c:(13|16):\[0-9]*: \[^\n\r]*store to misaligned address 0x\[0-9a-fA-F]* for type 'int', which requires 4 byte alignment.*" } */
 /* { dg-output "\[^\n\r]*\.c:23:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
 /* { dg-output "\[^\n\r]*\.c:(29|30):\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\[^\n\r]*\.c:30:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\[^\n\r]*\.c:31:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
 /* { dg-output "\[^\n\r]*\.c:37:\[0-9]*: \[^\n\r]*load of misaligned address 0x\[0-9a-fA-F]* for type 'long long int', which requires \[48] byte alignment" } */
diff --git gcc/testsuite/c-c++-common/ubsan/align-6.c gcc/testsuite/c-c++-common/ubsan/align-6.c
index e69de29..5521292 100644
--- gcc/testsuite/c-c++-common/ubsan/align-6.c
+++ gcc/testsuite/c-c++-common/ubsan/align-6.c
@@ -0,0 +1,33 @@ 
+/* Limit this to known non-strict alignment targets.  */
+/* { dg-do run { target { i?86-*-linux* x86_64-*-linux* } } } */
+/* { dg-options "-O -fsanitize=alignment -fsanitize-recover=alignment" } */
+
+struct S { int a; char b; long long c; short d[10]; };
+struct T { char a; long long b; };
+struct U { char a; int b; int c; long long d; struct S e; struct T f; } __attribute__((packed));
+struct V { long long a; struct S b; struct T c; struct U u; } v;
+
+__attribute__((noinline, noclone)) int
+foo (struct S *p)
+{
+  volatile int i;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  return p->a;
+}
+
+int
+main ()
+{
+  if (foo (&v.u.e))
+    __builtin_abort ();
+  return 0;
+}
+
+/* { dg-output "\.c:14:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:15:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:16:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:17:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-output "\.c:18:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
diff --git gcc/testsuite/c-c++-common/ubsan/align-7.c gcc/testsuite/c-c++-common/ubsan/align-7.c
index e69de29..4a18d8d 100644
--- gcc/testsuite/c-c++-common/ubsan/align-7.c
+++ gcc/testsuite/c-c++-common/ubsan/align-7.c
@@ -0,0 +1,32 @@ 
+/* Limit this to known non-strict alignment targets.  */
+/* { dg-do run { target { i?86-*-linux* x86_64-*-linux* } } } */
+/* { dg-options "-O -fsanitize=alignment -fno-sanitize-recover=alignment -fdump-tree-sanopt-details" } */
+/* { dg-shouldfail "ubsan" } */
+
+struct S { int a; char b; long long c; short d[10]; };
+struct T { char a; long long b; };
+struct U { char a; int b; int c; long long d; struct S e; struct T f; } __attribute__((packed));
+struct V { long long a; struct S b; struct T c; struct U u; } v;
+
+__attribute__((noinline, noclone)) int
+foo (struct S *p)
+{
+  volatile int i;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  return p->a;
+}
+
+int
+main ()
+{
+  if (foo (&v.u.e))
+    __builtin_abort ();
+  return 0;
+}
+
+/* { dg-output "\.c:15:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct S', which requires \[48] byte alignment.*" } */
+/* { dg-final { scan-tree-dump-times "Optimizing" 4 "sanopt"} } */
+/* { dg-final { cleanup-tree-dump "sanopt" } } */
diff --git gcc/testsuite/c-c++-common/ubsan/align-8.c gcc/testsuite/c-c++-common/ubsan/align-8.c
index e69de29..b930162 100644
--- gcc/testsuite/c-c++-common/ubsan/align-8.c
+++ gcc/testsuite/c-c++-common/ubsan/align-8.c
@@ -0,0 +1,31 @@ 
+/* Limit this to known non-strict alignment targets.  */
+/* { dg-do run { target { i?86-*-linux* x86_64-*-linux* } } } */
+/* { dg-options "-O -fsanitize=alignment -fsanitize-undefined-trap-on-error -fdump-tree-sanopt-details" } */
+/* { dg-shouldfail "ubsan" } */
+
+struct S { int a; char b; long long c; short d[10]; };
+struct T { char a; long long b; };
+struct U { char a; int b; int c; long long d; struct S e; struct T f; } __attribute__((packed));
+struct V { long long a; struct S b; struct T c; struct U u; } v;
+
+__attribute__((noinline, noclone)) int
+foo (struct S *p)
+{
+  volatile int i;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  i = p->a;
+  return p->a;
+}
+
+int
+main ()
+{
+  if (foo (&v.u.e))
+    __builtin_abort ();
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Optimizing" 4 "sanopt"} } */
+/* { dg-final { cleanup-tree-dump "sanopt" } } */
diff --git gcc/testsuite/g++.dg/ubsan/null-1.C gcc/testsuite/g++.dg/ubsan/null-1.C
index 83b3033..e1524b1 100644
--- gcc/testsuite/g++.dg/ubsan/null-1.C
+++ gcc/testsuite/g++.dg/ubsan/null-1.C
@@ -25,4 +25,6 @@  main (void)
 }
 
 // { dg-output "reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
 // { dg-output "\[^\n\r]*reference binding to null pointer of type 'const L'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
diff --git gcc/testsuite/g++.dg/ubsan/null-2.C gcc/testsuite/g++.dg/ubsan/null-2.C
index 0230c7c..88f387e 100644
--- gcc/testsuite/g++.dg/ubsan/null-2.C
+++ gcc/testsuite/g++.dg/ubsan/null-2.C
@@ -35,3 +35,5 @@  main (void)
 
 // { dg-output "\.C:26:\[0-9]*:\[\^\n\r]*member call on null pointer of type 'struct U'.*" }
 // { dg-output "\.C:29:\[0-9]*:\[\^\n\r]*member call on null pointer of type 'struct V'.*" }
+// { dg-output "\.C:31:\[0-9]*:\[\^\n\r]*member call on null pointer of type 'struct V'.*" }
+// { dg-output "\.C:33:\[0-9]*:\[\^\n\r]*member call on null pointer of type 'struct V'" }
diff --git gcc/testsuite/g++.dg/ubsan/null-3.C gcc/testsuite/g++.dg/ubsan/null-3.C
index e69de29..cd3716b 100644
--- gcc/testsuite/g++.dg/ubsan/null-3.C
+++ gcc/testsuite/g++.dg/ubsan/null-3.C
@@ -0,0 +1,20 @@ 
+// { dg-do run }
+// { dg-options "-fsanitize=null" }
+
+int
+main (void)
+{
+  int *p = 0;
+
+  int &r1 = *p;
+  int &r2 = *p;
+  int &r3 = *p;
+  int &r4 = *p;
+  int &r5 = *p;
+}
+
+// { dg-output "reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-output "\[^\n\r]*reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
diff --git gcc/testsuite/g++.dg/ubsan/null-4.C gcc/testsuite/g++.dg/ubsan/null-4.C
index e69de29..9cb04ef 100644
--- gcc/testsuite/g++.dg/ubsan/null-4.C
+++ gcc/testsuite/g++.dg/ubsan/null-4.C
@@ -0,0 +1,19 @@ 
+// { dg-do run }
+// { dg-options "-O -fsanitize=null -fno-sanitize-recover=null -fdump-tree-sanopt-details" }
+// { dg-shouldfail "ubsan" }
+
+int
+main (void)
+{
+  int *p = 0;
+
+  int &r1 = *p;
+  int &r2 = *p;
+  int &r3 = *p;
+  int &r4 = *p;
+  int &r5 = *p;
+}
+
+// { dg-output "reference binding to null pointer of type 'int'(\n|\r\n|\r)" }
+// { dg-final { scan-tree-dump-times "Optimizing" 4 "sanopt"} }
+// { dg-final { cleanup-tree-dump "sanopt" } }
diff --git gcc/testsuite/g++.dg/ubsan/null-5.C gcc/testsuite/g++.dg/ubsan/null-5.C
index e69de29..d8e4a68 100644
--- gcc/testsuite/g++.dg/ubsan/null-5.C
+++ gcc/testsuite/g++.dg/ubsan/null-5.C
@@ -0,0 +1,18 @@ 
+// { dg-do run }
+// { dg-options "-O -fsanitize=null -fsanitize-undefined-trap-on-error -fdump-tree-sanopt-details" }
+// { dg-shouldfail "ubsan" }
+
+int
+main (void)
+{
+  int *p = 0;
+
+  int &r1 = *p;
+  int &r2 = *p;
+  int &r3 = *p;
+  int &r4 = *p;
+  int &r5 = *p;
+}
+
+// { dg-final { scan-tree-dump-times "Optimizing" 4 "sanopt"} }
+// { dg-final { cleanup-tree-dump "sanopt" } }