This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]