search

staticanalysis / data-race-test

Automatically exported from code.google.com/p/data-race-test
0 stars 0 forks source link

tsanv2: instrumentation of p->x=0; p->y=0 #96

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
The following may be instrumented as a single memory access, however it's 
somewhat questionable:

struct P {
  int x;
  int y;
};

void foobar(struct P *p) {
  p->x = 0;
  p->y = 0;
}

void foobar(struct P *p) {
    2430:       53                      push   %rbx
    2431:       48 89 fb                mov    %rdi,%rbx
    2434:       48 8b 7c 24 08          mov    0x8(%rsp),%rdi
    2439:       e8 52 03 00 00          callq  2790 <__tsan_func_entry>
  p->x = 0;
    243e:       48 89 df                mov    %rbx,%rdi
    2441:       e8 2a 67 00 00          callq  8b70 <__tsan_write4>
    2446:       c7 03 00 00 00 00       movl   $0x0,(%rbx)
  p->y = 0;
    244c:       48 8d 7b 04             lea    0x4(%rbx),%rdi
    2450:       e8 1b 67 00 00          callq  8b70 <__tsan_write4>
    2455:       c7 43 04 00 00 00 00    movl   $0x0,0x4(%rbx)
}
    245c:       e8 6f 02 00 00          callq  26d0 <__tsan_func_exit>
    2461:       5b                      pop    %rbx
    2462:       c3                      retq

Original issue reported on code.google.com by dvyu...@google.com on 2 Apr 2012 at 11:39

GoogleCodeExporter commented 9 years ago 
questionable indeed. 
Here we basically need to check *almost* the same conditions as the existing 
compiler pass (in LLVM: load widening, which is part of GVN).
First of all, x should be 8-aligned.

But this means that if the conditions are true, such code should not survive 
after GVN and tsan instrumentation should not see it. 
And if GVN does not work we should fix it, not tsan.

Having said that, I can construct a test where GVN should not work, 
but tsan could widen the instrumentation:

struct S {
  double alignment;
  int x, y;
};         

int foo(S *s, int *a) {
  int x = s->x;        
  *a = 0;              
  int y = s->y;        
  return x + y;        
}                

here, s->x and s->y can be widened by tsan but can not be widened by GVN due to 
aliasing. 

Leaving this bug open, but not planing to act on it in near term.

Original comment by konstant...@gmail.com on 10 Apr 2012 at 10:54

GoogleCodeExporter commented 9 years ago 
One more data point: even when load widening may happen, 
it is not necessary beneficial and may not actually happen. 
But widening the tsan instrumentation is almost always good idea.

Original comment by konstant...@gmail.com on 10 Apr 2012 at 10:58

GoogleCodeExporter commented 9 years ago 
From dvyukov: 
I meant that it's questionable even for tsan. The problem is that all related 
accesses (even not widened) will fall into "intersect" slow-path rather than 
fast and expected "same size". Moreover it will trash shadow, now we have slot 
for x, y and for x+y. At some point in time I had precise classification of 
accesses size (same, larger, smaller, intersect, not intersect), but now we 
have only same, intersect and not intersect, so, for example, access to x won't 
be replaced with access to x+y; instead they will compete for shadow real 
estate.
It's quite difficult to predict effects of such things, so it must be postponed 
until after we have good representative benchmarks. Then such questions are 
easily answered.

Original comment by konstant...@gmail.com on 11 Apr 2012 at 3:19