[tsan] Small bugfix

Hi!

When I've tried to compile the attached testcase (I was trying to see
if tsan could discover the emutls.c data race), I got ICEs because
expr_ptr in certain cases wasn't is_gimple_val and thus was invalid to
pass it directly to a call as argument, fixed thusly.

Unfortunately, trying to compile it dynamically against libtsan.so
doesn't work (apparently it is using -fvisibility=hidden, but not
saying the public entry points have default visibility), compiling it
by hand statically against libtsan.a (we don't have -static-libtsan yet)
failed at runtime, complaining the binary isn't a PIE - can't it really
support normal executables?, and when compiled/linked as PIE, I got

	Jakub
#include <pthread.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>

pthread_key_t key;
pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;

struct S
{
  uintptr_t offset;
  size_t size;
};

struct T
{
  uintptr_t size;
  void **data[];
};

void
destroy (void *x)
{
  struct T *p = x;
  uintptr_t size = p->size;
  uintptr_t i;

  for (i = 0; i < size; ++i)
    free (p->data[i]);

  free (x);
}

void
init (void)
{
  if (pthread_key_create (&key, destroy))
    exit (0);
}

static uintptr_t size;

void *
foo (struct S *x)
{
  uintptr_t offset = x->offset;
  if (__builtin_expect (offset == 0, 0))
    {
      static pthread_once_t once = PTHREAD_ONCE_INIT;
      pthread_once (&once, init);
      pthread_mutex_lock (&lock);
      offset = x->offset;
      if (offset == 0)
	{
	  offset = ++size;
	  x->offset = offset;
	}
      pthread_mutex_unlock (&lock);
    }

  struct T *p = (struct T *) pthread_getspecific (key);
  if (__builtin_expect (p == NULL, 0))
    {
      uintptr_t s = offset + 2;
      p = (struct T *) calloc (s + 1, sizeof (void *));
      if (p == NULL)
	exit (0); 
      p->size = s;
      pthread_setspecific (key, (void *) p);
    }
  else if (__builtin_expect (offset > p->size, 0))
    {
      uintptr_t o = p->size;
      uintptr_t s = 2 * o;
      if (offset > s)
	s = offset + 2;
      p = realloc (p, (s + 1) * sizeof (void *));
      if (p == NULL)
	exit (0); 
      memset (p->data + o, 0, (s - o) * sizeof (void *));
      p->size = s;
      pthread_setspecific (key, (void *) p);
    }

  void *ret = p->data[offset - 1];
  if (__builtin_expect (ret == NULL, 0))
    {
      ret = malloc (x->size);
      if (ret == NULL)
	exit (0);
      p->data[offset - 1] = ret;
    }
  return ret;
}

struct S s[3] = { { 0, 5 }, { 0, 12 }, { 0, 21 } };

void *
tf (void *arg)
{
  (void) arg;
  foo (&s[0]);
  foo (&s[1]);
  foo (&s[0]);
  foo (&s[2]);
  return NULL;
}

int
main (void)
{
  pthread_t p[2];
  int i;
  for (i = 0; i < 5; i++)
    if (pthread_create (&p[i], NULL, tf, NULL))
      return 0;
  for (i = 0; i < 5; i++)
    pthread_join (p[i], NULL);
  return 0;
}

On Sat, Dec 1, 2012 at 4:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> When I've tried to compile the attached testcase (I was trying to see
> if tsan could discover the emutls.c data race), I got ICEs because
> expr_ptr in certain cases wasn't is_gimple_val and thus was invalid to
> pass it directly to a call as argument, fixed thusly.
>
> Unfortunately, trying to compile it dynamically against libtsan.so
> doesn't work (apparently it is using -fvisibility=hidden, but not
> saying the public entry points have default visibility),

Runtime needs to mark all interface functions as visibility("default"), right?


> compiling it
> by hand statically against libtsan.a (we don't have -static-libtsan yet)
> failed at runtime, complaining the binary isn't a PIE - can't it really
> support normal executables?

It's not trivial to do fast shadow memory mapping in this case.
Initially non pie builds ware not planned at. But I am starting to
think that I know how to do it.
I will try to look into it in next weeks.


> and when compiled/linked as PIE, I got
> ==================
> WARNING: ThreadSanitizer: thread leak (pid=31150)
>   Thread 3 (tid=31153, finished) created at:
>     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>     #1 main ??:0 (exe+0x00000000505f)
>
> ==================
> ==================
> WARNING: ThreadSanitizer: thread leak (pid=31150)
>   Thread 4 (tid=31155, finished) created at:
>     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>     #1 main ??:0 (exe+0x00000000505f)
>
> ==================
> ==================
> WARNING: ThreadSanitizer: thread leak (pid=31150)
>   Thread 5 (tid=31156, finished) created at:
>     #0 pthread_create ??:0 (exe+0x00000000e4dc)
>     #1 main ??:0 (exe+0x00000000505f)
>
> ==================
> ThreadSanitizer: reported 3 warnings
>
> which is probably not what I was expecting to see.

The thread leak reports are correct, right?
The race must be detectable. Can you show the code? The first thing to
check is that the memory accesses are instrumented. Also if you build
runtime with -DTSAN_DEBUG_OUTPUT=2 it will print all incoming events;
if you post the log most likely I will be able to say why the race is
not detected.


> 2012-12-01  Jakub Jelinek  <jakub@redhat.com>
>
>         * tsan.c (instrument_expr): If expr_ptr isn't a gimple val, first
>         store it into a SSA_NAME.
>
> --- gcc/tsan.c.jj       2012-11-30 19:17:13.000000000 +0100
> +++ gcc/tsan.c  2012-11-30 21:50:54.695392123 +0100
> @@ -93,10 +93,11 @@ is_vptr_store (gimple stmt, tree expr, b
>  static bool
>  instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write)
>  {
> -  tree base, rhs, expr_type, expr_ptr, builtin_decl;
> +  tree base, rhs, expr_ptr, builtin_decl;
>    basic_block bb;
>    HOST_WIDE_INT size;
>    gimple stmt, g;
> +  gimple_seq seq;
>    location_t loc;
>
>    size = int_size_in_bytes (TREE_TYPE (expr));
> @@ -139,21 +140,25 @@ instrument_expr (gimple_stmt_iterator gs
>    rhs = is_vptr_store (stmt, expr, is_write);
>    gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
>    expr_ptr = build_fold_addr_expr (unshare_expr (expr));
> -  if (rhs == NULL)
> +  seq = NULL;
> +  if (!is_gimple_val (expr_ptr))
>      {
> -      expr_type = TREE_TYPE (expr);
> -      while (TREE_CODE (expr_type) == ARRAY_TYPE)
> -       expr_type = TREE_TYPE (expr_type);
> -      size = int_size_in_bytes (expr_type);
> -      g = gimple_build_call (get_memory_access_decl (is_write, size),
> -                            1, expr_ptr);
> +      g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr_ptr), NULL),
> +                              expr_ptr);
> +      expr_ptr = gimple_assign_lhs (g);
> +      gimple_set_location (g, loc);
> +      gimple_seq_add_stmt_without_update (&seq, g);
>      }
> +  if (rhs == NULL)
> +    g = gimple_build_call (get_memory_access_decl (is_write, size),
> +                          1, expr_ptr);
>    else
>      {
>        builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
>        g = gimple_build_call (builtin_decl, 1, expr_ptr);
>      }
>    gimple_set_location (g, loc);
> +  gimple_seq_add_stmt_without_update (&seq, g);
>    /* Instrumentation for assignment of a function result
>       must be inserted after the call.  Instrumentation for
>       reads of function arguments must be inserted before the call.
> @@ -170,13 +175,13 @@ instrument_expr (gimple_stmt_iterator gs
>           bb = gsi_bb (gsi);
>           e = find_fallthru_edge (bb->succs);
>           if (e)
> -           gsi_insert_seq_on_edge_immediate (e, g);
> +           gsi_insert_seq_on_edge_immediate (e, seq);
>         }
>        else
> -       gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +       gsi_insert_seq_after (&gsi, seq, GSI_NEW_STMT);
>      }
>    else
> -    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> +    gsi_insert_before (&gsi, seq, GSI_SAME_STMT);
>
>    return true;
>  }
>
>         Jakub