Patchwork [asan] Fix for PR asan/56330

login
register
mail settings
Submitter Dodji Seketeli
Date Feb. 15, 2013, 8:54 p.m.
Message ID <874nhdb8n8.fsf@redhat.com>
Download mbox | patch
Permalink /patch/220848/
State New
Headers show

Comments

Dodji Seketeli - Feb. 15, 2013, 8:54 p.m.
Hello,

This PR uncovers an issue my latest optimization patch on Address
Sanitizer introduced.

This is in the context of instrumenting an access to a memory region,
like in:

    void
    foo (char *a, char *b, int s)
    {
      __builtin_memmove (a, b, s);
    }

In this case, asan has to instrument accesses to two memory regions:
[a a+s[ and [b b+s[.

The way asan does instrument the access to e.g [a a+s[ his is that it
instruments the access to the byte pointed to by the pointer 'a', and
the access to the byte pointed to by the pointer 'a+s-1'.

Now what happens when we want to avoid doing redundant
instrumentations like in:

    void
    bar (char *a, char *b, char *c)
    {
      __builtin_memmove (a, b, c[b[0]]);
    }

So here, we first instrument the access to the memory of b[0] (in the
expression c[b[0]]).  So in the course of instrumenting the access for
the memory region [b b+s[, we don't want to instrument the access to
the memory pointed by 'p', as that was already been instrumented for
the b[0] expression.  So we just instrument the access to memory
through b+s-1.

This is what I'd call 'partial memory region instrumentation'; it
happens in the function instrument_mem_region_access, and it was
basically corrupting the CFG because instrument_mem_region_access was
not correctly updating the statement iterator it receives in argument
so subsequent use of that iterator (to append instrumentation
statements) was leading to statements not being added to their correct
place.  The compiler crash reported in this PR is due to this CFG
corruption.

There is another underlying issue that can lead to runtime errors.
When doing partial memory region instrumentation in e.g the case of
the bar function above I was forgetting to enclose the instrumentation
statements of [a, a+s[ and [b, b+s[ into a if (len != 0) {} clause
(len being c[b[0]] here) for the cases where len is not a constant.
The patch addresses this too.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk, and
approved by Jakub in the audit trail of the PR at
http://gcc.gnu.org/PR56330.  I am about to commit in a short while.

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/asan.c                                         | 97 ++++++++++++----------
 .../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          | 22 +++++
 .../asan/no-redundant-instrumentation-8.c          | 14 ++++
 gcc/testsuite/c-c++-common/asan/pr56330.c          | 23 +++++
 8 files changed, 153 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
Jack Howarth - Feb. 15, 2013, 11:01 p.m.
On Fri, Feb 15, 2013 at 09:54:19PM +0100, Dodji Seketeli wrote:
> Hello,
> 
> This PR uncovers an issue my latest optimization patch on Address
> Sanitizer introduced.
> 
> This is in the context of instrumenting an access to a memory region,
> like in:
> 
>     void
>     foo (char *a, char *b, int s)
>     {
>       __builtin_memmove (a, b, s);
>     }
> 
> In this case, asan has to instrument accesses to two memory regions:
> [a a+s[ and [b b+s[.
> 
> The way asan does instrument the access to e.g [a a+s[ his is that it
> instruments the access to the byte pointed to by the pointer 'a', and
> the access to the byte pointed to by the pointer 'a+s-1'.
> 
> Now what happens when we want to avoid doing redundant
> instrumentations like in:
> 
>     void
>     bar (char *a, char *b, char *c)
>     {
>       __builtin_memmove (a, b, c[b[0]]);
>     }
> 
> So here, we first instrument the access to the memory of b[0] (in the
> expression c[b[0]]).  So in the course of instrumenting the access for
> the memory region [b b+s[, we don't want to instrument the access to
> the memory pointed by 'p', as that was already been instrumented for
> the b[0] expression.  So we just instrument the access to memory
> through b+s-1.
> 
> This is what I'd call 'partial memory region instrumentation'; it
> happens in the function instrument_mem_region_access, and it was
> basically corrupting the CFG because instrument_mem_region_access was
> not correctly updating the statement iterator it receives in argument
> so subsequent use of that iterator (to append instrumentation
> statements) was leading to statements not being added to their correct
> place.  The compiler crash reported in this PR is due to this CFG
> corruption.
> 
> There is another underlying issue that can lead to runtime errors.
> When doing partial memory region instrumentation in e.g the case of
> the bar function above I was forgetting to enclose the instrumentation
> statements of [a, a+s[ and [b, b+s[ into a if (len != 0) {} clause
> (len being c[b[0]] here) for the cases where len is not a constant.
> The patch addresses this too.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk, and
> approved by Jakub in the audit trail of the PR at
> http://gcc.gnu.org/PR56330.  I am about to commit in a short while.

On x86_64-apple-darwin12, this patch is producing a failure of...

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...

# /sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/gcc/testsuite/g++/../../xg++ -B/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/gcc/testsuite/g++/../../ /sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c  -fsanitize=address -g -fno-diagnostics-show-caret  -nostdinc++ -I/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/x86_64-apple-darwin12.2.0/libstdc++-v3/include/x86_64-apple-darwin12.2.0 -I/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/x86_64-apple-darwin12.2.0/libstdc++-v3/include -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/libsupc++ -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/include/backward -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/testsuite/util -fmessage-length=0  -O0  -fdump-tree-asan0  -S  -m64 -o no-redundant-instrumentation-7.s

> 
> 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/asan.c                                         | 97 ++++++++++++----------
>  .../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          | 22 +++++
>  .../asan/no-redundant-instrumentation-8.c          | 14 ++++
>  gcc/testsuite/c-c++-common/asan/pr56330.c          | 23 +++++
>  8 files changed, 153 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..aba409b
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
> @@ -0,0 +1,22 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +char e[200];
> +
> +struct S
> +{
> +  char a[100];
> +  char b[100];
> +} s;
> +
> +void
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memcmp (s.a, e, 100);
> +  __builtin_memcmp (s.a, e, 200);
> +}
> +
> +/* { 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
.text
Ltext0:
	.globl _e
	.zerofill __DATA,__pu_bss5,_e,256,5
	.globl _s
	.zerofill __DATA,__pu_bss5,_s,256,5
	.text
	.globl __Z3fooPiPcS0_
__Z3fooPiPcS0_:
LFB0:
	.file 1 "/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c"
	.loc 1 15 0
	pushq	%rbp
LCFI0:
	movq	%rsp, %rbp
LCFI1:
	movq	%rdi, -8(%rbp)
	movq	%rsi, -16(%rbp)
	movq	%rdx, -24(%rbp)
	.loc 1 18 0
	popq	%rbp
LCFI2:
	ret
LFE0:
	.cstring
	.align 3
LC0:
	.ascii "s (/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c)\0"
	.align 3
LC1:
	.ascii "e (/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c)\0"
	.data
	.align 5
LASAN0:
	.quad	_s
	.quad	200
	.quad	256
	.quad	LC0
	.quad	0
	.quad	_e
	.quad	200
	.quad	256
	.quad	LC1
	.quad	0
	.text
__GLOBAL__sub_D_00099_0_no_redundant_instrumentation_7.c:
LFB1:
	.loc 1 18 0
	pushq	%rbp
LCFI3:
	movq	%rsp, %rbp
LCFI4:
	.loc 1 18 0
	movl	$2, %esi
	leaq	LASAN0(%rip), %rdi
	call	___asan_unregister_globals
	popq	%rbp
LCFI5:
	ret
LFE1:
__GLOBAL__sub_I_00099_1_no_redundant_instrumentation_7.c:
LFB2:
	.loc 1 18 0
	pushq	%rbp
LCFI6:
	movq	%rsp, %rbp
LCFI7:
	.loc 1 18 0
	call	___asan_init_v1
	movl	$2, %esi
	leaq	LASAN0(%rip), %rdi
	call	___asan_register_globals
	popq	%rbp
LCFI8:
	ret
LFE2:
	.section __DWARF,__debug_frame,regular,debug
Lsection__debug_frame:
Lframe0:
	.set L$set$0,LECIE0-LSCIE0
	.long L$set$0
LSCIE0:
	.long	0xffffffff
	.byte	0x1
	.ascii "\0"
	.byte	0x1
	.byte	0x78
	.byte	0x10
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.byte	0x90
	.byte	0x1
	.align 3
LECIE0:
LSFDE0:
	.set L$set$1,LEFDE0-LASFDE0
	.long L$set$1
LASFDE0:
	.set L$set$2,Lframe0-Lsection__debug_frame
	.long L$set$2
	.quad	LFB0
	.set L$set$3,LFE0-LFB0
	.quad L$set$3
	.byte	0x4
	.set L$set$4,LCFI0-LFB0
	.long L$set$4
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$5,LCFI1-LCFI0
	.long L$set$5
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$6,LCFI2-LCFI1
	.long L$set$6
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE0:
LSFDE2:
	.set L$set$7,LEFDE2-LASFDE2
	.long L$set$7
LASFDE2:
	.set L$set$8,Lframe0-Lsection__debug_frame
	.long L$set$8
	.quad	LFB1
	.set L$set$9,LFE1-LFB1
	.quad L$set$9
	.byte	0x4
	.set L$set$10,LCFI3-LFB1
	.long L$set$10
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$11,LCFI4-LCFI3
	.long L$set$11
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$12,LCFI5-LCFI4
	.long L$set$12
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE2:
LSFDE4:
	.set L$set$13,LEFDE4-LASFDE4
	.long L$set$13
LASFDE4:
	.set L$set$14,Lframe0-Lsection__debug_frame
	.long L$set$14
	.quad	LFB2
	.set L$set$15,LFE2-LFB2
	.quad L$set$15
	.byte	0x4
	.set L$set$16,LCFI6-LFB2
	.long L$set$16
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$17,LCFI7-LCFI6
	.long L$set$17
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$18,LCFI8-LCFI7
	.long L$set$18
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE4:
	.section __TEXT,__eh_frame,coalesced,no_toc+strip_static_syms+live_support
EH_frame1:
	.set L$set$19,LECIE1-LSCIE1
	.long L$set$19
LSCIE1:
	.long	0
	.byte	0x1
	.ascii "zR\0"
	.byte	0x1
	.byte	0x78
	.byte	0x10
	.byte	0x1
	.byte	0x10
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.byte	0x90
	.byte	0x1
	.align 3
LECIE1:
LSFDE7:
	.set L$set$20,LEFDE7-LASFDE7
	.long L$set$20
LASFDE7:
	.long	LASFDE7-EH_frame1
	.quad	LFB0-.
	.set L$set$21,LFE0-LFB0
	.quad L$set$21
	.byte	0
	.byte	0x4
	.set L$set$22,LCFI0-LFB0
	.long L$set$22
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$23,LCFI1-LCFI0
	.long L$set$23
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$24,LCFI2-LCFI1
	.long L$set$24
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE7:
LSFDE9:
	.set L$set$25,LEFDE9-LASFDE9
	.long L$set$25
LASFDE9:
	.long	LASFDE9-EH_frame1
	.quad	LFB1-.
	.set L$set$26,LFE1-LFB1
	.quad L$set$26
	.byte	0
	.byte	0x4
	.set L$set$27,LCFI3-LFB1
	.long L$set$27
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$28,LCFI4-LCFI3
	.long L$set$28
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$29,LCFI5-LCFI4
	.long L$set$29
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE9:
LSFDE11:
	.set L$set$30,LEFDE11-LASFDE11
	.long L$set$30
LASFDE11:
	.long	LASFDE11-EH_frame1
	.quad	LFB2-.
	.set L$set$31,LFE2-LFB2
	.quad L$set$31
	.byte	0
	.byte	0x4
	.set L$set$32,LCFI6-LFB2
	.long L$set$32
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$33,LCFI7-LCFI6
	.long L$set$33
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$34,LCFI8-LCFI7
	.long L$set$34
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE11:
	.text
Letext0:
	.section __DWARF,__debug_info,regular,debug
Lsection__debug_info:
Ldebug_info0:
	.long	0x302
	.word	0x2
	.set L$set$35,Ldebug_abbrev0-Lsection__debug_abbrev
	.long L$set$35
	.byte	0x8
	.byte	0x1
	.ascii "GNU C++ 4.8.0 20130215 (experimental) -fPIC -feliminate-unused-debug-symbols -mmacosx-version-min=10.8.2 -m64 -mtune=core2 -g -O0 -fsanitize=address -fmessage-length=0\0"
	.byte	0x4
	.ascii "/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c\0"
	.ascii "/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/gcc/testsuite/g++\0"
	.quad	Ltext0
	.quad	Letext0
	.set L$set$36,Ldebug_line0-Lsection__debug_line
	.long L$set$36
	.byte	0x1
	.byte	0x2
	.ascii "S\0"
	.byte	0xc8
	.byte	0x1
	.byte	0x7
	.long	0x1a7
	.byte	0x3
	.ascii "a\0"
	.byte	0x1
	.byte	0x9
	.long	0x1a7
	.byte	0x2
	.byte	0x23
	.byte	0
	.byte	0x3
	.ascii "b\0"
	.byte	0x1
	.byte	0xa
	.long	0x1a7
	.byte	0x2
	.byte	0x23
	.byte	0x64
	.byte	0
	.byte	0x4
	.long	0x1c3
	.long	0x1b7
	.byte	0x5
	.long	0x1b7
	.byte	0x63
	.byte	0
	.byte	0x6
	.byte	0x8
	.byte	0x7
	.ascii "sizetype\0"
	.byte	0x6
	.byte	0x1
	.byte	0x6
	.ascii "char\0"
	.byte	0x7
	.byte	0x1
	.ascii "foo\0"
	.byte	0x1
	.byte	0xe
	.ascii "_Z3fooPiPcS0_\0"
	.quad	LFB0
	.quad	LFE0
	.set L$set$37,LLST0-Lsection__debug_loc
	.long L$set$37
	.long	0x21e
	.byte	0x8
	.ascii "a\0"
	.byte	0x1
	.byte	0xe
	.long	0x21e
	.byte	0x2
	.byte	0x91
	.byte	0x68
	.byte	0x8
	.ascii "b\0"
	.byte	0x1
	.byte	0xe
	.long	0x22b
	.byte	0x2
	.byte	0x91
	.byte	0x60
	.byte	0x8
	.ascii "c\0"
	.byte	0x1
	.byte	0xe
	.long	0x22b
	.byte	0x2
	.byte	0x91
	.byte	0x58
	.byte	0
	.byte	0x9
	.byte	0x8
	.long	0x224
	.byte	0x6
	.byte	0x4
	.byte	0x5
	.ascii "int\0"
	.byte	0x9
	.byte	0x8
	.long	0x1c3
	.byte	0x4
	.long	0x1c3
	.long	0x241
	.byte	0x5
	.long	0x1b7
	.byte	0xc7
	.byte	0
	.byte	0xa
	.ascii "e\0"
	.byte	0x1
	.byte	0x5
	.long	0x231
	.byte	0x1
	.byte	0x9
	.byte	0x3
	.quad	_e
	.byte	0xa
	.ascii "s\0"
	.byte	0x1
	.byte	0xb
	.long	0x184
	.byte	0x1
	.byte	0x9
	.byte	0x3
	.quad	_s
	.byte	0xb
	.ascii "_GLOBAL__sub_D_00099_0_no_redundant_instrumentation_7.c\0"
	.byte	0x1
	.quad	LFB1
	.quad	LFE1
	.set L$set$38,LLST1-Lsection__debug_loc
	.long L$set$38
	.byte	0xb
	.ascii "_GLOBAL__sub_I_00099_1_no_redundant_instrumentation_7.c\0"
	.byte	0x1
	.quad	LFB2
	.quad	LFE2
	.set L$set$39,LLST2-Lsection__debug_loc
	.long L$set$39
	.byte	0
	.section __DWARF,__debug_abbrev,regular,debug
Lsection__debug_abbrev:
Ldebug_abbrev0:
	.byte	0x1
	.byte	0x11
	.byte	0x1
	.byte	0x25
	.byte	0x8
	.byte	0x13
	.byte	0xb
	.byte	0x3
	.byte	0x8
	.byte	0x1b
	.byte	0x8
	.byte	0x11
	.byte	0x1
	.byte	0x12
	.byte	0x1
	.byte	0x10
	.byte	0x6
	.byte	0xb4,0x42
	.byte	0xc
	.byte	0
	.byte	0
	.byte	0x2
	.byte	0x13
	.byte	0x1
	.byte	0x3
	.byte	0x8
	.byte	0xb
	.byte	0xb
	.byte	0x3a
	.byte	0xb
	.byte	0x3b
	.byte	0xb
	.byte	0x1
	.byte	0x13
	.byte	0
	.byte	0
	.byte	0x3
	.byte	0xd
	.byte	0
	.byte	0x3
	.byte	0x8
	.byte	0x3a
	.byte	0xb
	.byte	0x3b
	.byte	0xb
	.byte	0x49
	.byte	0x13
	.byte	0x38
	.byte	0xa
	.byte	0
	.byte	0
	.byte	0x4
	.byte	0x1
	.byte	0x1
	.byte	0x49
	.byte	0x13
	.byte	0x1
	.byte	0x13
	.byte	0
	.byte	0
	.byte	0x5
	.byte	0x21
	.byte	0
	.byte	0x49
	.byte	0x13
	.byte	0x2f
	.byte	0xb
	.byte	0
	.byte	0
	.byte	0x6
	.byte	0x24
	.byte	0
	.byte	0xb
	.byte	0xb
	.byte	0x3e
	.byte	0xb
	.byte	0x3
	.byte	0x8
	.byte	0
	.byte	0
	.byte	0x7
	.byte	0x2e
	.byte	0x1
	.byte	0x3f
	.byte	0xc
	.byte	0x3
	.byte	0x8
	.byte	0x3a
	.byte	0xb
	.byte	0x3b
	.byte	0xb
	.byte	0x87,0x40
	.byte	0x8
	.byte	0x11
	.byte	0x1
	.byte	0x12
	.byte	0x1
	.byte	0x40
	.byte	0x6
	.byte	0x1
	.byte	0x13
	.byte	0
	.byte	0
	.byte	0x8
	.byte	0x5
	.byte	0
	.byte	0x3
	.byte	0x8
	.byte	0x3a
	.byte	0xb
	.byte	0x3b
	.byte	0xb
	.byte	0x49
	.byte	0x13
	.byte	0x2
	.byte	0xa
	.byte	0
	.byte	0
	.byte	0x9
	.byte	0xf
	.byte	0
	.byte	0xb
	.byte	0xb
	.byte	0x49
	.byte	0x13
	.byte	0
	.byte	0
	.byte	0xa
	.byte	0x34
	.byte	0
	.byte	0x3
	.byte	0x8
	.byte	0x3a
	.byte	0xb
	.byte	0x3b
	.byte	0xb
	.byte	0x49
	.byte	0x13
	.byte	0x3f
	.byte	0xc
	.byte	0x2
	.byte	0xa
	.byte	0
	.byte	0
	.byte	0xb
	.byte	0x2e
	.byte	0
	.byte	0x3
	.byte	0x8
	.byte	0x34
	.byte	0xc
	.byte	0x11
	.byte	0x1
	.byte	0x12
	.byte	0x1
	.byte	0x40
	.byte	0x6
	.byte	0
	.byte	0
	.byte	0
	.section __DWARF,__debug_loc,regular,debug
Lsection__debug_loc:
Ldebug_loc0:
LLST0:
	.set L$set$40,LFB0-Ltext0
	.quad L$set$40
	.set L$set$41,LCFI0-Ltext0
	.quad L$set$41
	.word	0x2
	.byte	0x77
	.byte	0x8
	.set L$set$42,LCFI0-Ltext0
	.quad L$set$42
	.set L$set$43,LCFI1-Ltext0
	.quad L$set$43
	.word	0x2
	.byte	0x77
	.byte	0x10
	.set L$set$44,LCFI1-Ltext0
	.quad L$set$44
	.set L$set$45,LCFI2-Ltext0
	.quad L$set$45
	.word	0x2
	.byte	0x76
	.byte	0x10
	.set L$set$46,LCFI2-Ltext0
	.quad L$set$46
	.set L$set$47,LFE0-Ltext0
	.quad L$set$47
	.word	0x2
	.byte	0x77
	.byte	0x8
	.quad	0
	.quad	0
LLST1:
	.set L$set$48,LFB1-Ltext0
	.quad L$set$48
	.set L$set$49,LCFI3-Ltext0
	.quad L$set$49
	.word	0x2
	.byte	0x77
	.byte	0x8
	.set L$set$50,LCFI3-Ltext0
	.quad L$set$50
	.set L$set$51,LCFI4-Ltext0
	.quad L$set$51
	.word	0x2
	.byte	0x77
	.byte	0x10
	.set L$set$52,LCFI4-Ltext0
	.quad L$set$52
	.set L$set$53,LCFI5-Ltext0
	.quad L$set$53
	.word	0x2
	.byte	0x76
	.byte	0x10
	.set L$set$54,LCFI5-Ltext0
	.quad L$set$54
	.set L$set$55,LFE1-Ltext0
	.quad L$set$55
	.word	0x2
	.byte	0x77
	.byte	0x8
	.quad	0
	.quad	0
LLST2:
	.set L$set$56,LFB2-Ltext0
	.quad L$set$56
	.set L$set$57,LCFI6-Ltext0
	.quad L$set$57
	.word	0x2
	.byte	0x77
	.byte	0x8
	.set L$set$58,LCFI6-Ltext0
	.quad L$set$58
	.set L$set$59,LCFI7-Ltext0
	.quad L$set$59
	.word	0x2
	.byte	0x77
	.byte	0x10
	.set L$set$60,LCFI7-Ltext0
	.quad L$set$60
	.set L$set$61,LCFI8-Ltext0
	.quad L$set$61
	.word	0x2
	.byte	0x76
	.byte	0x10
	.set L$set$62,LCFI8-Ltext0
	.quad L$set$62
	.set L$set$63,LFE2-Ltext0
	.quad L$set$63
	.word	0x2
	.byte	0x77
	.byte	0x8
	.quad	0
	.quad	0
	.section __DWARF,__debug_pubnames,regular,debug
Lsection__debug_pubnames:
	.long	0x9a
	.word	0x2
	.set L$set$64,Ldebug_info0-Lsection__debug_info
	.long L$set$64
	.long	0x306
	.long	0x1cb
	.ascii "foo\0"
	.long	0x241
	.ascii "e\0"
	.long	0x255
	.ascii "s\0"
	.long	0x269
	.ascii "_GLOBAL__sub_D_00099_0_no_redundant_instrumentation_7.c\0"
	.long	0x2b7
	.ascii "_GLOBAL__sub_I_00099_1_no_redundant_instrumentation_7.c\0"
	.long	0
	.section __DWARF,__debug_pubtypes,regular,debug
Lsection__debug_pubtypes:
	.long	0x32
	.word	0x2
	.set L$set$65,Ldebug_info0-Lsection__debug_info
	.long L$set$65
	.long	0x306
	.long	0x1b7
	.ascii "sizetype\0"
	.long	0x1c3
	.ascii "char\0"
	.long	0x184
	.ascii "S\0"
	.long	0x224
	.ascii "int\0"
	.long	0
	.section __DWARF,__debug_aranges,regular,debug
Lsection__debug_aranges:
	.long	0x2c
	.word	0x2
	.set L$set$66,Ldebug_info0-Lsection__debug_info
	.long L$set$66
	.byte	0x8
	.byte	0
	.word	0
	.word	0
	.quad	Ltext0
	.set L$set$67,Letext0-Ltext0
	.quad L$set$67
	.quad	0
	.quad	0
	.section __DWARF,__debug_line,regular,debug
Lsection__debug_line:
Ldebug_line0:
	.section __DWARF,__debug_str,regular,debug
Lsection__debug_str:
	.mod_init_func
	.align 3
	.quad	__GLOBAL__sub_I_00099_1_no_redundant_instrumentation_7.c
	.mod_term_func
	.align 3
	.quad	__GLOBAL__sub_D_00099_0_no_redundant_instrumentation_7.c
	.constructor
	.destructor
	.align 1
	.subsections_via_symbols
Jakub Jelinek - Feb. 16, 2013, 8:37 a.m.
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.

	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..aba409b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
@@ -0,0 +1,22 @@ 
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+char e[200];
+
+struct S
+{
+  char a[100];
+  char b[100];
+} s;
+
+void
+foo  (int *a, char *b, char *c)
+{
+  __builtin_memcmp (s.a, e, 100);
+  __builtin_memcmp (s.a, e, 200);
+}
+
+/* { 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]]);
+}