[asan] Handle bitfields in asan

Hi!

This patch tries to handle bitfield accesses as if it were writes/reads
from the corresponding DECL_BIT_FIELD_REPRESENTATIVE.
Richard, does it make sense this way (the code will take ADDR_EXPR
of the COMPONENT_REF with the DECL_BIT_FIELD_REPRESENTATIVE and use
the size of the representative if it decides to instrument it.

Fixes the first 4 asan_test.C failures.  Ok for trunk?

2012-12-05  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (instrument_derefs): Handle bitfield COMPONENT_REFs
	accesses as reads/writes to their DECL_BIT_FIELD_REPRESENTATIVE.


	Jakub

On Wed, 5 Dec 2012, Jakub Jelinek wrote:

> Hi!
> 
> This patch tries to handle bitfield accesses as if it were writes/reads
> from the corresponding DECL_BIT_FIELD_REPRESENTATIVE.
> Richard, does it make sense this way (the code will take ADDR_EXPR
> of the COMPONENT_REF with the DECL_BIT_FIELD_REPRESENTATIVE and use
> the size of the representative if it decides to instrument it.
> 
> Fixes the first 4 asan_test.C failures.  Ok for trunk?

It makes sense apart from the address-taking - is this done early
enough to not disrupt aliasing / TREE_ADDRESSABLE?  Thus, before
gimplifying?

Thanks,
Richard.

> 2012-12-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* asan.c (instrument_derefs): Handle bitfield COMPONENT_REFs
> 	accesses as reads/writes to their DECL_BIT_FIELD_REPRESENTATIVE.
> 
> --- gcc/asan.c.jj	2012-12-05 15:30:56.000000000 +0100
> +++ gcc/asan.c	2012-12-05 17:00:56.957641944 +0100
> @@ -792,9 +792,6 @@ instrument_derefs (gimple_stmt_iterator
>        || (unsigned HOST_WIDE_INT) size_in_bytes - 1 >= 16)
>      return;
>  
> -  /* For now just avoid instrumenting bit field acceses.
> -     Fixing it is doable, but expected to be messy.  */
> -
>    HOST_WIDE_INT bitsize, bitpos;
>    tree offset;
>    enum machine_mode mode;
> @@ -803,7 +800,17 @@ instrument_derefs (gimple_stmt_iterator
>  		       &mode, &unsignedp, &volatilep, false);
>    if (bitpos % (size_in_bytes * BITS_PER_UNIT)
>        || bitsize != size_in_bytes * BITS_PER_UNIT)
> -    return;
> +    {
> +      if (TREE_CODE (t) == COMPONENT_REF
> +	  && DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (t, 1)) != NULL_TREE)
> +	{
> +	  tree repr = DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (t, 1));
> +	  instrument_derefs (iter, build3 (COMPONENT_REF, TREE_TYPE (repr),
> +					   TREE_OPERAND (t, 0), repr,
> +					   NULL_TREE), location, is_store);
> +	}
> +      return;
> +    }
>  
>    base = build_fold_addr_expr (t);
>    build_check_stmt (location, base, iter, /*before_p=*/true,
> 
> 	Jakub
> 
>

On Wed, Dec 05, 2012 at 05:10:56PM +0100, Richard Biener wrote:
> > This patch tries to handle bitfield accesses as if it were writes/reads
> > from the corresponding DECL_BIT_FIELD_REPRESENTATIVE.
> > Richard, does it make sense this way (the code will take ADDR_EXPR
> > of the COMPONENT_REF with the DECL_BIT_FIELD_REPRESENTATIVE and use
> > the size of the representative if it decides to instrument it.
> > 
> > Fixes the first 4 asan_test.C failures.  Ok for trunk?
> 
> It makes sense apart from the address-taking - is this done early
> enough to not disrupt aliasing / TREE_ADDRESSABLE?  Thus, before
> gimplifying?

It is done far later than that (except for -Og and -O0 right after PRE),
but I thought that isn't a big deal, it takes address for everything
to be instrumented, and AFAIK tree-ssa-operands.c will take care of
calling mark_addressable on it if needed.  Asan doesn't actually dereference
that pointer ever, all it does is compute from that address the shadow
address (addr >> 3) + (1L << 44) or similar, and dereferences that.
And for asan, we want to instrument
  char p[32];
  for (int i = 0; i < n; i++)
    p[i] = 0;
even when p isn't TREE_ADDRESSABLE (ditto for bitfields).  So, all in all,
even if the address is taken, it will never escape because of the asan
instrumentation.
Doesn't e.g. ivopts also cause non-addressable vars to become addressable,
if it replaces ARRAY_REFs with taking address of the array before the loop
and using MEM_REF/TARGET_MEM_REF in the loop?

	Jakub

On Wed, 5 Dec 2012, Jakub Jelinek wrote:

> On Wed, Dec 05, 2012 at 05:10:56PM +0100, Richard Biener wrote:
> > > This patch tries to handle bitfield accesses as if it were writes/reads
> > > from the corresponding DECL_BIT_FIELD_REPRESENTATIVE.
> > > Richard, does it make sense this way (the code will take ADDR_EXPR
> > > of the COMPONENT_REF with the DECL_BIT_FIELD_REPRESENTATIVE and use
> > > the size of the representative if it decides to instrument it.
> > > 
> > > Fixes the first 4 asan_test.C failures.  Ok for trunk?
> > 
> > It makes sense apart from the address-taking - is this done early
> > enough to not disrupt aliasing / TREE_ADDRESSABLE?  Thus, before
> > gimplifying?
> 
> It is done far later than that (except for -Og and -O0 right after PRE),
> but I thought that isn't a big deal, it takes address for everything
> to be instrumented, and AFAIK tree-ssa-operands.c will take care of
> calling mark_addressable on it if needed.  Asan doesn't actually dereference
> that pointer ever, all it does is compute from that address the shadow
> address (addr >> 3) + (1L << 44) or similar, and dereferences that.
> And for asan, we want to instrument
>   char p[32];
>   for (int i = 0; i < n; i++)
>     p[i] = 0;
> even when p isn't TREE_ADDRESSABLE (ditto for bitfields).  So, all in all,
> even if the address is taken, it will never escape because of the asan
> instrumentation.

Ok.

> Doesn't e.g. ivopts also cause non-addressable vars to become addressable,
> if it replaces ARRAY_REFs with taking address of the array before the loop
> and using MEM_REF/TARGET_MEM_REF in the loop?

Yes, but it also doesn't make them escape (autopar does, for example,
which is why it has to re-set points-to info ...).

Richard.

Jakub Jelinek <jakub@redhat.com> writes:

> 2012-12-05  Jakub Jelinek  <jakub@redhat.com>
>
> 	* asan.c (instrument_derefs): Handle bitfield COMPONENT_REFs
> 	accesses as reads/writes to their DECL_BIT_FIELD_REPRESENTATIVE.

After Richi's comments in this thread, I guess this is OK to me.