Jakub Jelinek <ja...@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
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]]);
+}
--
Dodji
