[tsan] instrument_expr improvements

Hi!

As discussed earlier on mailing list and on IRC with Richard,
pt_solution_includes can be used for testing of whether var
might escape current function (for -O0 unfortunately returns
true even for !may_be_aliased automatic vars, so I'm testing
for that too), and there are IMHO tons of completely unnecessary tests
and restrictions what we want to instrument and what not.
instrument_expr is guarded with gimple_store_p or
gimple_assign_load_p, so it shouldn't see SSA_NAMEs, and many other
ops, basically just DECL_Ps, handled_components, MEM_REF or TARGET_MEM_REF
where for handled_components the base address is one of DECL_P, MEM_REF
or TARGET_MEM_REF.

I'm actually surprised that insturment_expr handles also is_gimple_call
in it, yet nothing calls it for calls, I bet for calls we also want to call
it for gimple_store_p calls.

Dmitry, could you please test this on something where the previous tsan
code has been tested on (if anything)?  All I've done was eyeball a few
short testcases.

TSAN will badly need similar optimization pass to what ASAN needs (after
deferring expansion of the shadow memory checks), e.g. var++ right now
results in __tsan_read4 (&var); followed soon by __tsan_write4 (&var);.
With no intervening calls (we could ignore many string/memory builtins
I guess) and no intervening atomics it should be fine to just use
__tsan_write4 (&var); for that, right?  Similarly for multiple accesses
to the same var, where first use dominates the other accesses and there
are no intervening calls.

As for bitfields, I think we want to use DECL_BIT_FIELD_REPRESENTATIVE.

2012-11-29  Jakub Jelinek  <jakub@redhat.com>

	* tsan.c (is_load_of_const_p): Removed.
	(instrument_expr): Use result of get_inner_reference
	instead of get_base_address, avoid some unnecessary tests,
	use !pt_solution_includes and !may_be_aliased tests to
	check whether base might escape current function.


	Jakub

On Thu, Nov 29, 2012 at 4:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As discussed earlier on mailing list and on IRC with Richard,
> pt_solution_includes can be used for testing of whether var
> might escape current function (for -O0 unfortunately returns
> true even for !may_be_aliased automatic vars, so I'm testing
> for that too),

This optimization would be great.
-O0 performance is completely irrelevant, it just needs to work. tsan
should be used with -O1.


> and there are IMHO tons of completely unnecessary tests
> and restrictions what we want to instrument and what not.
> instrument_expr is guarded with gimple_store_p or
> gimple_assign_load_p, so it shouldn't see SSA_NAMEs, and many other
> ops, basically just DECL_Ps, handled_components, MEM_REF or TARGET_MEM_REF
> where for handled_components the base address is one of DECL_P, MEM_REF
> or TARGET_MEM_REF.
>
> I'm actually surprised that insturment_expr handles also is_gimple_call
> in it, yet nothing calls it for calls, I bet for calls we also want to call
> it for gimple_store_p calls.

Originally I wrote it for slightly different instrumentation with no
prior knowledge about gcc. The the pass placement was changes, then
instrumentation was changed. It has a lot of artefacts.


> Dmitry, could you please test this on something where the previous tsan
> code has been tested on (if anything)?  All I've done was eyeball a few
> short testcases.

OK, I will try to build it and test on something. I was not yet tested
it on anything at all.

> TSAN will badly need similar optimization pass to what ASAN needs (after
> deferring expansion of the shadow memory checks), e.g. var++ right now
> results in __tsan_read4 (&var); followed soon by __tsan_write4 (&var);.
> With no intervening calls (we could ignore many string/memory builtins
> I guess) and no intervening atomics it should be fine to just use
> __tsan_write4 (&var); for that, right?

Right.

> Similarly for multiple accesses
> to the same var, where first use dominates the other accesses and there
> are no intervening calls.

We can do it in some cases. However, it dominating access is read, and
the second is conditional write, then we need to preserve both.





> As for bitfields, I think we want to use DECL_BIT_FIELD_REPRESENTATIVE.
>
> 2012-11-29  Jakub Jelinek  <jakub@redhat.com>
>
>         * tsan.c (is_load_of_const_p): Removed.
>         (instrument_expr): Use result of get_inner_reference
>         instead of get_base_address, avoid some unnecessary tests,
>         use !pt_solution_includes and !may_be_aliased tests to
>         check whether base might escape current function.
>
> --- gcc/tsan.c.jj       2012-11-23 15:27:38.000000000 +0100
> +++ gcc/tsan.c  2012-11-29 12:41:26.816857288 +0100
> @@ -84,65 +84,18 @@ is_vptr_store (gimple stmt, tree expr, b
>    return NULL;
>  }
>
> -/* Checks as to whether EXPR refers to constant var/field/param.
> -   Don't bother to instrument them.  */
> -
> -static bool
> -is_load_of_const_p (tree expr, bool is_write)
> -{
> -  if (is_write)
> -    return false;
> -  if (TREE_CODE (expr) == COMPONENT_REF)
> -    expr = TREE_OPERAND (expr, 1);
> -  if (TREE_CODE (expr) == VAR_DECL
> -      || TREE_CODE (expr) == PARM_DECL
> -      || TREE_CODE (expr) == FIELD_DECL)
> -    {
> -      if (TREE_READONLY (expr))
> -       return true;
> -    }
> -  return false;
> -}
> -
>  /* Instruments EXPR if needed. If any instrumentation is inserted,
>     return true.  */
>
>  static bool
>  instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
>  {
> -  enum tree_code tcode;
>    tree base, rhs, expr_type, expr_ptr, builtin_decl;
>    basic_block bb;
>    HOST_WIDE_INT size;
>    gimple stmt, g;
>    location_t loc;
>
> -  base = get_base_address (expr);
> -  if (base == NULL_TREE
> -      || TREE_CODE (base) == SSA_NAME
> -      || TREE_CODE (base) == STRING_CST)
> -    return false;
> -
> -  tcode = TREE_CODE (expr);
> -
> -  /* Below are things we do not instrument
> -     (no possibility of races or not implemented yet).  */
> -  if (/* Compiler-emitted artificial variables.  */
> -      (DECL_P (expr) && DECL_ARTIFICIAL (expr))
> -      /* The var does not live in memory -> no possibility of races.  */
> -      || (tcode == VAR_DECL
> -         && !TREE_ADDRESSABLE (expr)
> -         && TREE_STATIC (expr) == 0)
> -      /* Not implemented.  */
> -      || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE
> -      /* Not implemented.  */
> -      || tcode == CONSTRUCTOR
> -      /* Not implemented.  */
> -      || tcode == PARM_DECL
> -      /* Load of a const variable/parameter/field.  */
> -      || is_load_of_const_p (expr, is_write))
> -    return false;
> -
>    size = int_size_in_bytes (TREE_TYPE (expr));
>    if (size == -1)
>      return false;
> @@ -153,18 +106,29 @@ instrument_expr (gimple_stmt_iterator gs
>    tree offset;
>    enum machine_mode mode;
>    int volatilep = 0, unsignedp = 0;
> -  get_inner_reference (expr, &bitsize, &bitpos, &offset,
> -                      &mode, &unsignedp, &volatilep, false);
> -  if (bitpos % (size * BITS_PER_UNIT)
> -      || bitsize != size * BITS_PER_UNIT)
> +  base = get_inner_reference (expr, &bitsize, &bitpos, &offset,
> +                             &mode, &unsignedp, &volatilep, false);
> +
> +  /* No need to instrument accesses to decls that don't escape,
> +     they can't escape to other threads then.  */
> +  if (DECL_P (base))
> +    {
> +      struct pt_solution pt;
> +      memset (&pt, 0, sizeof (pt));
> +      pt.escaped = 1;
> +      pt.ipa_escaped = flag_ipa_pta != 0;
> +      pt.nonlocal = 1;
> +      if (!pt_solution_includes (&pt, base))
> +       return false;
> +      if (!is_global_var (base) && !may_be_aliased (base))
> +       return false;
> +    }
> +
> +  if (TREE_READONLY (base))
>      return false;
>
> -  /* TODO: handle other case: ARRAY_RANGE_REF.  */
> -  if (tcode != ARRAY_REF
> -      && tcode != VAR_DECL
> -      && tcode != COMPONENT_REF
> -      && tcode != INDIRECT_REF
> -      && tcode != MEM_REF)
> +  if (bitpos % (size * BITS_PER_UNIT)
> +      || bitsize != size * BITS_PER_UNIT)
>      return false;
>
>    stmt = gsi_stmt (gsi);
>
>         Jakub

On Thu, 29 Nov 2012, Jakub Jelinek wrote:

> Hi!
> 
> As discussed earlier on mailing list and on IRC with Richard,
> pt_solution_includes can be used for testing of whether var
> might escape current function (for -O0 unfortunately returns
> true even for !may_be_aliased automatic vars, so I'm testing
> for that too), and there are IMHO tons of completely unnecessary tests
> and restrictions what we want to instrument and what not.
> instrument_expr is guarded with gimple_store_p or
> gimple_assign_load_p, so it shouldn't see SSA_NAMEs, and many other
> ops, basically just DECL_Ps, handled_components, MEM_REF or TARGET_MEM_REF
> where for handled_components the base address is one of DECL_P, MEM_REF
> or TARGET_MEM_REF.
> 
> I'm actually surprised that insturment_expr handles also is_gimple_call
> in it, yet nothing calls it for calls, I bet for calls we also want to call
> it for gimple_store_p calls.
> 
> Dmitry, could you please test this on something where the previous tsan
> code has been tested on (if anything)?  All I've done was eyeball a few
> short testcases.
> 
> TSAN will badly need similar optimization pass to what ASAN needs (after
> deferring expansion of the shadow memory checks), e.g. var++ right now
> results in __tsan_read4 (&var); followed soon by __tsan_write4 (&var);.
> With no intervening calls (we could ignore many string/memory builtins
> I guess) and no intervening atomics it should be fine to just use
> __tsan_write4 (&var); for that, right?  Similarly for multiple accesses
> to the same var, where first use dominates the other accesses and there
> are no intervening calls.
> 
> As for bitfields, I think we want to use DECL_BIT_FIELD_REPRESENTATIVE.

From a middle-end POV this looks ok.

Thanks,
Richard.

> 2012-11-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tsan.c (is_load_of_const_p): Removed.
> 	(instrument_expr): Use result of get_inner_reference
> 	instead of get_base_address, avoid some unnecessary tests,
> 	use !pt_solution_includes and !may_be_aliased tests to
> 	check whether base might escape current function.
> 
> --- gcc/tsan.c.jj	2012-11-23 15:27:38.000000000 +0100
> +++ gcc/tsan.c	2012-11-29 12:41:26.816857288 +0100
> @@ -84,65 +84,18 @@ is_vptr_store (gimple stmt, tree expr, b
>    return NULL;
>  }
>  
> -/* Checks as to whether EXPR refers to constant var/field/param.
> -   Don't bother to instrument them.  */
> -
> -static bool
> -is_load_of_const_p (tree expr, bool is_write)
> -{
> -  if (is_write)
> -    return false;
> -  if (TREE_CODE (expr) == COMPONENT_REF)
> -    expr = TREE_OPERAND (expr, 1);
> -  if (TREE_CODE (expr) == VAR_DECL
> -      || TREE_CODE (expr) == PARM_DECL
> -      || TREE_CODE (expr) == FIELD_DECL)
> -    {
> -      if (TREE_READONLY (expr))
> -	return true;
> -    }
> -  return false;
> -}
> -
>  /* Instruments EXPR if needed. If any instrumentation is inserted,
>     return true.  */
>  
>  static bool
>  instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
>  {
> -  enum tree_code tcode;
>    tree base, rhs, expr_type, expr_ptr, builtin_decl;
>    basic_block bb;
>    HOST_WIDE_INT size;
>    gimple stmt, g;
>    location_t loc;
>  
> -  base = get_base_address (expr);
> -  if (base == NULL_TREE
> -      || TREE_CODE (base) == SSA_NAME
> -      || TREE_CODE (base) == STRING_CST)
> -    return false;
> -
> -  tcode = TREE_CODE (expr);
> -
> -  /* Below are things we do not instrument
> -     (no possibility of races or not implemented yet).  */
> -  if (/* Compiler-emitted artificial variables.  */
> -      (DECL_P (expr) && DECL_ARTIFICIAL (expr))
> -      /* The var does not live in memory -> no possibility of races.  */
> -      || (tcode == VAR_DECL
> -	  && !TREE_ADDRESSABLE (expr)
> -	  && TREE_STATIC (expr) == 0)
> -      /* Not implemented.  */
> -      || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE
> -      /* Not implemented.  */
> -      || tcode == CONSTRUCTOR
> -      /* Not implemented.  */
> -      || tcode == PARM_DECL
> -      /* Load of a const variable/parameter/field.  */
> -      || is_load_of_const_p (expr, is_write))
> -    return false;
> -
>    size = int_size_in_bytes (TREE_TYPE (expr));
>    if (size == -1)
>      return false;
> @@ -153,18 +106,29 @@ instrument_expr (gimple_stmt_iterator gs
>    tree offset;
>    enum machine_mode mode;
>    int volatilep = 0, unsignedp = 0;
> -  get_inner_reference (expr, &bitsize, &bitpos, &offset,
> -		       &mode, &unsignedp, &volatilep, false);
> -  if (bitpos % (size * BITS_PER_UNIT)
> -      || bitsize != size * BITS_PER_UNIT)
> +  base = get_inner_reference (expr, &bitsize, &bitpos, &offset,
> +			      &mode, &unsignedp, &volatilep, false);
> +
> +  /* No need to instrument accesses to decls that don't escape,
> +     they can't escape to other threads then.  */
> +  if (DECL_P (base))
> +    {
> +      struct pt_solution pt;
> +      memset (&pt, 0, sizeof (pt));
> +      pt.escaped = 1;
> +      pt.ipa_escaped = flag_ipa_pta != 0;
> +      pt.nonlocal = 1;
> +      if (!pt_solution_includes (&pt, base))
> +	return false;
> +      if (!is_global_var (base) && !may_be_aliased (base))
> +	return false;
> +    }
> +
> +  if (TREE_READONLY (base))
>      return false;
>  
> -  /* TODO: handle other case: ARRAY_RANGE_REF.  */
> -  if (tcode != ARRAY_REF
> -      && tcode != VAR_DECL
> -      && tcode != COMPONENT_REF
> -      && tcode != INDIRECT_REF
> -      && tcode != MEM_REF)
> +  if (bitpos % (size * BITS_PER_UNIT)
> +      || bitsize != size * BITS_PER_UNIT)
>      return false;
>  
>    stmt = gsi_stmt (gsi);
> 
> 	Jakub
> 
>