Re: [PATCH 2/2] [asan] Avoid instrumenting duplicated memory access in the same basic block

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Feb 12, 2013 at 03:19:37PM +0100, Dodji Seketeli wrote:
> gcc/
> 	* Makefile.in (asan.o): Add new dependency on hash-table.h
> 	* asan.c (struct asan_mem_ref, struct mem_ref_hasher): New types.
> 	(asan_mem_ref_init, asan_mem_ref_get_end, get_mem_ref_hash_table)
> 	(has_stmt_been_instrumented_p, empty_mem_ref_hash_table)
> 	(free_mem_ref_resources, has_mem_ref_been_instrumented)
> 	(has_stmt_been_instrumented_p, update_mem_ref_hash_table)
> 	(get_mem_ref_of_assignment): New functions.
> 	(get_mem_refs_of_builtin_call): Extract from
> 	instrument_builtin_call and tweak a little bit to make it fit with
> 	the new signature.
> 	(instrument_builtin_call): Use the new
> 	get_mem_refs_of_builtin_call.  Use gimple_call_builtin_p instead
> 	of is_gimple_builtin_call.
> 	(instrument_derefs, instrument_mem_region_access): Insert the
> 	instrumented memory reference into the hash table.
> 	(maybe_instrument_assignment): Renamed instrument_assignment into
> 	this, and change it to advance the iterator when instrumentation
> 	actually happened and return true in that case.  This makes it
> 	homogeneous with maybe_instrument_assignment, and thus give a
> 	chance to callers to be more 'regular'.
> 	(transform_statements): Clear the memory reference hash table
> 	whenever we enter a new BB, when we cross a function call, or when
> 	we are done transforming statements.  Use
> 	maybe_instrument_assignment instead of instrumentation.  No more
> 	need to special case maybe_instrument_assignment and advance the
> 	iterator after calling it; it's now handled just like
> 	maybe_instrument_call.  Update comment.

Ok.  Just some testsuite nits.

> gcc/testsuite/
> 
> 	* c-c++-common/asan/no-redundant-instrumentation-1.c: New test.
> 	* testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c: Likewise.
> 	* testsuite/c-c++-common/asan/no-redundant-instrumentation-3.c: Likewise.
> 	* testsuite/c-c++-common/asan/inc.c: Likewise.

> diff --git a/gcc/testsuite/c-c++-common/asan/inc.c b/gcc/testsuite/c-c++-common/asan/inc.c
> new file mode 100644
> index 0000000..09ad176
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/inc.c
> @@ -0,0 +1,20 @@
> +/* { dg-options "-fdump-tree-asan0" }
> +   { dg-do compile }  */

I think generally the style used in the gcc testsuite is for C tests:
/* { dg-options "..." } */
/* { dg-do compile } */
and for C++ // comments.
I.e. don't merge multiple lines into one comments, no extra spaces.
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report" 1 "asan0" } }  */
> +
> +/* { dg-final { scan-tree-dump "__builtin___asan_report_load4" "asan0" } }  */

No need for the empty line in between that.

> 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
> new file mode 100644
> index 0000000..8c6cca4
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
> @@ -0,0 +1,67 @@
> +/* This tests that when faced with two references to the same memory
> +   location in the same basic block, the second reference should not
> +   be instrumented by the Address Sanitizer

Here I'd end with a full stop, i.e. Address Sanitizer.  */
> +
> +   { dg-options "-fdump-tree-asan0" }
> +   { dg-do compile }
> + */

And the above would be one comment per one line style again.
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 7 "asan0" } }
> +
> +   { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 2 "asan0" }  }
> + */

Again, both one line comments, and no empty line in between.

> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c
> @@ -0,0 +1,26 @@
> +/* This tests that when faced with two references to the same memory
> +   location in the same basic block, the second reference should not
> +   be instrumented by the Address Sanitizer.  But in case of access to
> +   overlapping regions we must be precise.
> +
> +   { dg-options "-fdump-tree-asan0" }
> +   { dg-do compile }
> + */

Ditto.
> +
> +int
> +main ()
> +{
> +  char tab[5];
> +
> +  /* Here, we instrument the access at offset 0 and access at offset
> +     4.  */
> +  __builtin_memset (tab, 1, sizeof (tab));
> +  /* We instrumented access at offset 0 above already, so only access
> +     at offset 3 is instrumented.  */
> +  __builtin_memset (tab, 1, 3);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 3 "asan0" } }
> +
> +   { dg-final { scan-tree-dump-times "__builtin___asan_report" 3 "asan0" }  }
> + */

Ditto.

...

	Jakub