Patchwork [asan] Fix for PR asan/56330

login
register
mail settings
Submitter Dodji Seketeli
Date Feb. 16, 2013, 9:15 a.m.
Message ID <87fw0waacd.fsf@redhat.com>
Download mbox | patch
Permalink /patch/220952/
State New
Headers show

Comments

Dodji Seketeli - Feb. 16, 2013, 9:15 a.m.
Jakub Jelinek <jakub@redhat.com> writes:

> On Sat, Feb 16, 2013 at 09:44:51AM +0100, Dodji Seketeli wrote:
>> Right.  My first bootstrap actually caught this, I updated the patch
>> locally to just modify that test and sent out the wrong one.  Sorry for
>> this.  This is the patch that actually passed bootstrap.
>
> Ok.  Actually, please change gcc/testsuite/c-c++-common/asan/pr56330.c
> to the same thing,

Oops, right.  Done below.

> I've just verified it ICEs the same way without the patch
> even when it is
> int
> foo (void)
> {
>   int d = __builtin_memcmp (s.a, e, 100);
>   d += __builtin_memcmp (s.a, e, 200);
>   return d;
> }

Yes, I have just verified this too, thanks.

>
> (while with
>   __builtin_memcpy (s.a, e, 100);
>   __builtin_memcpy (s.a, e, 200);
> it doesn't ICE without the patch).

Hmmh, interesting.

> While pr56330.c test look to be redundant, it is at least tortured at all
> compilation options, so it tests something that the other tests don't.

Yes, that is why I kept it in the set of tests.

> No need to rebootstrap/retest.

I re-ran the tests locally w/o bootstrap though.  I am going to
commit this now.  Thanks.

gcc/
	* asan.c (get_mem_refs_of_builtin_call): White space and style
	cleanup.
	(instrument_mem_region_access): Do not forget to always put
	instrumentation of the of 'base' and 'base + len' in a "if (len !=
	0) statement, even for cases where either 'base' or 'base + len'
	are not instrumented -- because they have been previously
	instrumented.  Simplify the logic by putting all the statements
	instrument 'base + len' inside a sequence, and then insert that
	sequence right before the current insertion point.  Then, to
	instrument 'base + len', just get an iterator on that statement.
	And do not forget to update the pointer to iterator the function
	received as argument.

gcc/testsuite/

	* c-c++-common/asan/no-redundant-instrumentation-4.c: New test file.
	* c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise.
	* c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise.
	* c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise.
	* c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise.
	* c-c++-common/asan/pr56330.c: Likewise.
	* c-c++-common/asan/no-redundant-instrumentation-1.c (test1):
	Ensure the size argument of __builtin_memcpy is a constant.
---
 gcc/ChangeLog                                      | 16 ++++
 gcc/asan.c                                         | 97 ++++++++++++----------
 gcc/testsuite/ChangeLog                            | 12 +++
 .../asan/no-redundant-instrumentation-1.c          |  2 +-
 .../asan/no-redundant-instrumentation-4.c          | 13 +++
 .../asan/no-redundant-instrumentation-5.c          | 13 +++
 .../asan/no-redundant-instrumentation-6.c          | 14 ++++
 .../asan/no-redundant-instrumentation-7.c          | 23 +++++
 .../asan/no-redundant-instrumentation-8.c          | 14 ++++
 gcc/testsuite/c-c++-common/asan/pr56330.c          | 24 ++++++
 10 files changed, 183 insertions(+), 45 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
 create mode 100644 gcc/testsuite/c-c++-common/asan/pr56330.c

Patch

diff --git a/gcc/asan.c b/gcc/asan.c
index a569479..67236a9 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -747,20 +747,17 @@  get_mem_refs_of_builtin_call (const gimple call,
 
       got_reference_p = true;
     }
-    else
-      {
-	if (dest)
-	  {
-	    dst->start = dest;
-	    dst->access_size = access_size;
-	    *dst_len = NULL_TREE;
-	    *dst_is_store = is_store;
-	    *dest_is_deref = true;
-	    got_reference_p = true;
-	  }
-      }
+  else if (dest)
+    {
+      dst->start = dest;
+      dst->access_size = access_size;
+      *dst_len = NULL_TREE;
+      *dst_is_store = is_store;
+      *dest_is_deref = true;
+      got_reference_p = true;
+    }
 
-    return got_reference_p;
+  return got_reference_p;
 }
 
 /* Return true iff a given gimple statement has been instrumented.
@@ -1535,8 +1532,15 @@  instrument_mem_region_access (tree base, tree len,
 
   /* If the beginning of the memory region has already been
      instrumented, do not instrument it.  */
-  if (has_mem_ref_been_instrumented (base, 1))
-    goto after_first_instrumentation;
+  bool start_instrumented = has_mem_ref_been_instrumented (base, 1);
+
+  /* If the end of the memory region has already been instrumented, do
+     not instrument it. */
+  tree end = asan_mem_ref_get_end (base, len);
+  bool end_instrumented = has_mem_ref_been_instrumented (end, 1);
+
+  if (start_instrumented && end_instrumented)
+    return;
 
   if (!is_gimple_constant (len))
     {
@@ -1562,37 +1566,39 @@  instrument_mem_region_access (tree base, tree len,
 
       /* The 'then block' of the 'if (len != 0) condition is where
 	 we'll generate the asan instrumentation code now.  */
-      gsi = gsi_start_bb (then_bb);
+      gsi = gsi_last_bb (then_bb);
     }
 
-  /* Instrument the beginning of the memory region to be accessed,
-     and arrange for the rest of the intrumentation code to be
-     inserted in the then block *after* the current gsi.  */
-  build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
-
-  if (then_bb)
-    /* We are in the case where the length of the region is not
-       constant; so instrumentation code is being generated in the
-       'then block' of the 'if (len != 0) condition.  Let's arrange
-       for the subsequent instrumentation statements to go in the
-       'then block'.  */
-    gsi = gsi_last_bb (then_bb);
-  else
-    *iter = gsi;
-
-  update_mem_ref_hash_table (base, 1);
+  if (!start_instrumented)
+    {
+      /* Instrument the beginning of the memory region to be accessed,
+	 and arrange for the rest of the intrumentation code to be
+	 inserted in the then block *after* the current gsi.  */
+      build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
+
+      if (then_bb)
+	/* We are in the case where the length of the region is not
+	   constant; so instrumentation code is being generated in the
+	   'then block' of the 'if (len != 0) condition.  Let's arrange
+	   for the subsequent instrumentation statements to go in the
+	   'then block'.  */
+	gsi = gsi_last_bb (then_bb);
+      else
+        {
+          *iter = gsi;
+	  /* Don't remember this access as instrumented, if length
+	     is unknown.  It might be zero and not being actually
+	     instrumented, so we can't rely on it being instrumented.  */
+          update_mem_ref_hash_table (base, 1);
+	}
+    }
 
- after_first_instrumentation:
+  if (end_instrumented)
+    return;
 
   /* We want to instrument the access at the end of the memory region,
      which is at (base + len - 1).  */
 
-  /* If the end of the memory region has already been instrumented, do
-     not instrument it. */
-  tree end = asan_mem_ref_get_end (base, len);
-  if (has_mem_ref_been_instrumented (end, 1))
-    return;
-
   /* offset = len - 1;  */
   len = unshare_expr (len);
   tree offset;
@@ -1639,8 +1645,6 @@  instrument_mem_region_access (tree base, tree len,
 				  base, NULL);
   gimple_set_location (region_end, location);
   gimple_seq_add_stmt_without_update (&seq, region_end);
-  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
-  gsi_prev (&gsi);
 
   /* _2 = _1 + offset;  */
   region_end =
@@ -1649,13 +1653,18 @@  instrument_mem_region_access (tree base, tree len,
 				  gimple_assign_lhs (region_end),
 				  offset);
   gimple_set_location (region_end, location);
-  gsi_insert_after (&gsi, region_end, GSI_NEW_STMT);
+  gimple_seq_add_stmt_without_update (&seq, region_end);
+  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
 
   /* instrument access at _2;  */
+  gsi = gsi_for_stmt (region_end);
   build_check_stmt (location, gimple_assign_lhs (region_end),
 		    &gsi, /*before_p=*/false, is_store, 1);
 
-  update_mem_ref_hash_table (end, 1);
+  if (then_bb == NULL)
+    update_mem_ref_hash_table (end, 1);
+
+  *iter = gsi_for_stmt (gsi_stmt (*iter));
 }
 
 /* Instrument the call (to the builtin strlen function) pointed to by
@@ -1783,7 +1792,7 @@  instrument_builtin_call (gimple_stmt_iterator *iter)
 	    }
 	  else if (src0_len || src1_len || dest_len)
 	    {
-	      if (src0.start)
+	      if (src0.start != NULL_TREE)
 		instrument_mem_region_access (src0.start, src0_len,
 					      iter, loc, /*is_store=*/false);
 	      if (src1.start != NULL_TREE)
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
index cc98fdb..6cf6441 100644
--- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
@@ -45,7 +45,7 @@  test1 ()
   /* There are 2 calls to __builtin___asan_report_store1 and 2 calls
      to __builtin___asan_report_load1 to instrument the store to
      (subset of) the memory region of tab.  */
-  __builtin_memcpy (&tab[1], foo, sizeof (tab) - 1);
+  __builtin_memcpy (&tab[1], foo, 3);
 
   /* This should not generate a __builtin___asan_report_load1 because
      the reference to tab[1] has been already instrumented above.  */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
new file mode 100644
index 0000000..b2e7284
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
@@ -0,0 +1,13 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo  (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[c[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 1 "asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
new file mode 100644
index 0000000..ead3f58
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
@@ -0,0 +1,13 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo  (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[b[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "asan0" } } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
new file mode 100644
index 0000000..e4691bc
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
@@ -0,0 +1,14 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo  (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[c[0]]);
+  __builtin_memmove (c, b, a[b[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 5 "asan0" } } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
new file mode 100644
index 0000000..075e9cf
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
@@ -0,0 +1,23 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+char e[200];
+
+struct S
+{
+  char a[100];
+  char b[100];
+} s;
+
+int
+foo  (int *a, char *b, char *c)
+{
+  int d = __builtin_memcmp (s.a, e, 100);
+  d += __builtin_memcmp (s.a, e, 200);
+  return d;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 6 "asan0" } } */
+/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
new file mode 100644
index 0000000..38ea7a2
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
@@ -0,0 +1,14 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+char
+foo  (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[b[0]]);
+  return c[0] + b[0];
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/pr56330.c b/gcc/testsuite/c-c++-common/asan/pr56330.c
new file mode 100644
index 0000000..25759f4
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr56330.c
@@ -0,0 +1,24 @@ 
+/* PR sanitizer/56330 */
+/* { dg-do compile } */
+
+char e[200];
+
+struct S
+{
+  char a[100];
+  char b[100];
+} s;
+
+int
+foo (void)
+{
+  int i = __builtin_memcmp (s.a, e, 100);
+  i += __builtin_memcmp (s.a, e, 200);
+  return i;
+}
+
+void
+bar (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[b[0]]);
+}