Patchwork [asan] Fix for PR asan/56330

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

Comments

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

> On Fri, Feb 15, 2013 at 06:01:05PM -0500, Jack Howarth wrote:
>> On Fri, Feb 15, 2013 at 09:54:19PM +0100, Dodji Seketeli wrote:
>> FAIL: c-c++-common/asan/no-redundant-instrumentation-7.c  -O0   scan-tree-dump-times asan0 "__builtin___asan_report_load" 6
>> 
>> at both -m32 and -m64. The no-redundant-instrumentation-7.s from the failing -m64
>> test case is attached, generated with...
>
> I think
> void
> foo  (int *a, char *b, char *c)
> {
>   __builtin_memcmp (s.a, e, 100);
>   __builtin_memcmp (s.a, e, 200);
> }
> is problematic, for -O1 both calls would be definitely removed, because they
> are pure, and even at -O0 I guess they might be expanded into nothing.
> Perhaps
> int
> foo ()
> {
>   int i = __builtin_memcmp (s.a, e, 100);
>   i += __builtin_memcmp (s.a, e, 200);
>   return i;
> }
> or similar would work better.  And for pr56330.c testcase, which is there to
> verify that we don't ICE on it and I believe there was important that both
> builtins are adjacent, perhaps it should be __builtin_memcpy instead.

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.

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          | 23 +++++
 10 files changed, 182 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
Jakub Jelinek - Feb. 16, 2013, 8:53 a.m.
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, 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;
}

(while with
  __builtin_memcpy (s.a, e, 100);
  __builtin_memcpy (s.a, e, 200);
it doesn't ICE without the patch).
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.

No need to rebootstrap/retest.

	Jakub

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..fcb6ab6
--- /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..18f1065
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr56330.c
@@ -0,0 +1,23 @@ 
+/* PR sanitizer/56330 */
+/* { dg-do compile } */
+
+char e[200];
+
+struct S
+{
+  char a[100];
+  char b[100];
+} s;
+
+void
+foo (void)
+{
+  __builtin_memcmp (s.a, e, 100);
+  __builtin_memcmp (s.a, e, 200);
+}
+
+void
+bar (int *a, char *b, char *c)
+{
+  __builtin_memmove (c, b, a[b[0]]);
+}