|
From: Julian S. <js...@ac...> - 2010-01-27 16:24:10
|
Konstantin, I'm trying to annotate a user-implemented barrier, using http://code.google.com/p/google-perftools/source/browse/trunk/src/base/dynamic_annotations.h specifically ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER like this void barrier_wait ( barrier* b ) { ANNOTATE_HAPPENS_BEFORE(b); // the barrier implementation ANNOTATE_HAPPENS_AFTER(b); } But this doesn't work well, and I think it's not clear what semantics you intended for these two. This situation I can understand: T1: AH_BEFORE(obj) T2: AH_AFTER(obj) then we get an edge T1->T2 But what about this? T1: AH_BEFORE(obj) T2: AH_BEFORE(obj) T3: AH_AFTER(obj) Should we get edges T1->T3 and T2->T3, or only T2->T3 ? IOW, does a call AH_BEFORE(obj) cause obj to "forget" about any previous AH_BEFORE calls on it? It seems to me that to annotate a barrier properly requires generating two edges in this example, not just one. (Plus then it requires a way to tell obj to "forget everything" when the barrier is reinitialised, so that subsequent uses of it do not end up generating edges back to previous uses.) J |
|
From: Konstantin S. <kon...@gm...> - 2010-01-27 14:17:55
|
On Wed, Jan 27, 2010 at 3:41 PM, Julian Seward <js...@ac...> wrote: > > Konstantin, > > I'm trying to annotate a user-implemented barrier, using > > > http://code.google.com/p/google-perftools/source/browse/trunk/src/base/dynamic_annotations.h > specifically ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER > > like this > > void barrier_wait ( barrier* b ) > { > ANNOTATE_HAPPENS_BEFORE(b); > // the barrier implementation > ANNOTATE_HAPPENS_AFTER(b); > } > this is exactly how I handle barrier. > > But this doesn't work well, and I think it's not clear what semantics > you intended for these two. > > This situation I can understand: > > T1: AH_BEFORE(obj) > T2: AH_AFTER(obj) > > then we get an edge T1->T2 > > But what about this? > > T1: AH_BEFORE(obj) > T2: AH_BEFORE(obj) > T3: AH_AFTER(obj) > > Should we get edges T1->T3 and T2->T3, Yes, we will get two edges. The h-before events are never forgotten. (ThreadSanitizer forgets them when it flushes the entire state) > or only T2->T3 ? IOW, does > a call AH_BEFORE(obj) cause obj to "forget" about any previous > AH_BEFORE calls on it? > > It seems to me that to annotate a barrier properly requires generating > two edges in this example, not just one. (Plus then it requires > a way to tell obj to "forget everything" when the barrier is reinitialised, > so that subsequent uses of it do not end up generating edges back to > previous uses.) > That would be possible by adding yet another annotation (ANNOTATE_FORGET_HB_EDGES(obj)) but I never found this really useful, at least in ThreadSanitizer. --kcc > > J > |
|
From: Julian S. <js...@ac...> - 2010-01-27 16:24:07
|
> > But this doesn't work well, and I think it's not clear what semantics
> > you intended for these two.
> >
> > This situation I can understand:
> >
> > T1: AH_BEFORE(obj)
> > T2: AH_AFTER(obj)
> >
> > then we get an edge T1->T2
> >
> > But what about this?
> >
> > T1: AH_BEFORE(obj)
> > T2: AH_BEFORE(obj)
> > T3: AH_AFTER(obj)
> >
> > Should we get edges T1->T3 and T2->T3,
>
> Yes, we will get two edges.
> The h-before events are never forgotten. (ThreadSanitizer forgets them when
> it flushes the entire state)
I don't think it is good that the behaviour AH_BEFORE is hardwired
in this way. It makes the annotation set inflexible. What do you
think of the following proposal?
AH_BEFORE_OVERWRITE(obj)
forget everything in obj, then acquire a h-b dep from caller
AH_BEFORE_ACCUMULATE(obj)
merge in a h-b dep from caller; don't forget history
so your AH_BEFORE == AH_BEFORE_ACCUMULATE
AH_AFTER(obj)
acquire h-b edges from obj (unchanged)
A_FORGET(obj)
forget everything
You may not find A_FORGET and AH_BEFORE_OVERWRITE useful, but at least
there presence (+ clarification of the semantics) makes it more
flexible and controllable.
Also, I'd like to propose new annotations for barriers. We can annotate
barriers in terms of the above 4, but that hardwires the assumption that
the underlying checker has a concept of h-b edges. I'd prefer a set
of macros which merely specify the state of the barrier:
B_RESET(bar)
state that nobody is waiting for the barrier
B_ARRIVE(bar)
i have arrived at the barrier
B_DEPART(bar, Bool i_am_the_last_to_depart)
i have left the barrier, and give an indication of whether or
not I am the last to leave. (If so, the checker would
probably want to behave like B_RESET).
J
|
|
From: Julian S. <js...@ac...> - 2010-01-27 16:54:54
|
> Also, I'd like to propose new annotations for barriers. We can annotate
> barriers in terms of the above 4, but that hardwires the assumption that
> the underlying checker has a concept of h-b edges. I'd prefer a set
> of macros which merely specify the state of the barrier:
>
> B_RESET(bar)
> state that nobody is waiting for the barrier
>
> B_ARRIVE(bar)
> i have arrived at the barrier
>
> B_DEPART(bar, Bool i_am_the_last_to_depart)
> i have left the barrier, and give an indication of whether or
> not I am the last to leave. (If so, the checker would
> probably want to behave like B_RESET).
Actually, even with those, I think it is impossible to guarantee a correct
annotation in the worst-case race condition. In the worst-case condition
we need an arbitrary number of VTSs associated with the barrier, and also
to track how many threads are waiting at the barrier.
This is because it could be that threads leaving the "real" barrier
call/mechanism stall and do not get to the B_DEPART request (at which
point their VTSs are updated) before a whole different set of threads arrive
at the barrier, pass through it, add their own set of edges to it. Then
the original threads finally get to their B_DEPART requests, at which
point they get the edges of these "overtaking" threads incorrectly
added.
IOW we need to deal in the worst case with an arbitrary delay between
a thread leaving the barrier proper and having its state updated at
the B_DEPART request.
Hence I am now thinking instead of the following annotations:
B_INITIALISE(bar, int capacity)
initialise; state capacity
void* abstract_handle = B_ARRIVE(bar)
arrive at barrier. obtain abstract handle (a VTS*, essentially) from
which we need to update when leaving
B_DEPART(bar, abstract_handle)
depart from barrier, updating our VTS using the one we were given
by B_ARRIVE
---------------------
One other thing. I think, for the semantics of the annotations to make
sense generally, you need to say that, although the checker may run the
threads concurrently, it must process each annotation atomically. It
sounds like hair splitting, I know, but .. it's hard to see what the
effect would be if processing of annotations was allowed to happen
concurrently.
J
|
|
From: Bart V. A. <bva...@ac...> - 2010-01-28 07:32:49
|
On Wed, Jan 27, 2010 at 6:09 PM, Julian Seward <js...@ac...> wrote: > > [ ... ] > > IOW we need to deal in the worst case with an arbitrary delay between > a thread leaving the barrier proper and having its state updated at > the B_DEPART request. > > Hence I am now thinking instead of the following annotations: > > B_INITIALISE(bar, int capacity) > initialise; state capacity > > void* abstract_handle = B_ARRIVE(bar) > arrive at barrier. obtain abstract handle (a VTS*, essentially) from > which we need to update when leaving > > B_DEPART(bar, abstract_handle) > depart from barrier, updating our VTS using the one we were given > by B_ARRIVE The above looks a lot better to me than instrumenting user-implemented barriers via AH_BEFORE() and AH_AFTER(). While the latter approach is sufficient for suppressing false positives triggered by a user barrier implementation, these is a high risk that instrumenting user-implemented barriers via AH_BEFORE() and AH_AFTER() will suppress real races because these primitives introduce more inter-thread ordering than barriers. The above proposal doesn't have that disadvantage. By the way, the "abstract_handle" is unnecessary in my opinion. Since B_DEPART() will always be invoked from the same thread as the corresponding B_ARRIVE() call, a threading tool can figure out which B_DEPART() corresponds to B_ARRIVE() call by keeping a limited amount of state information. Bart. |
|
From: Julian S. <js...@ac...> - 2010-01-28 09:40:48
|
> By the way, the "abstract_handle" is unnecessary in my opinion. Since > B_DEPART() will always be invoked from the same thread as the > corresponding B_ARRIVE() call, a threading tool can figure out which > B_DEPART() corresponds to B_ARRIVE() call by keeping a limited amount > of state information. Yes, I think you're right. The extra information is: for each thread, the current barrier we're associated with, and, for that barrier, the VTS we must update our state by, when leaving. I think that would make the implementation more complex, though. Using the abstract_handles, the checker only needs to keep track of data per-barrier. With your "completely automatic" proposal it would have to also have some per-thread data. J |
|
From: Konstantin S. <kon...@gm...> - 2010-01-28 07:47:28
|
I agree that the current annotations used to implement barrier support in ThreadSanitizer *may* *in theory* hide some races. But I would like to see an example where it really hides them. And then, I would like to see a *realistic* example. :) I agree that AH_BEFORE_OVERWRITE and A_FORGET might be needed in some cases. (but again, I'd like to see a *realistic* test where the current annotations hide a race) But the problem with the annotations is that we already have too many of them and the users are sometimes confused. Also, are we talking here about implementing barrier support inside a tool (helgrind, drd, tsan, etc) or about exporting these annotations to users so that they can use it for their custom barriers? One other thing. I think, for the semantics of the annotations to make sense generally, you need to say that, although the checker may run the threads concurrently, it must process each annotation atomically. It sounds like hair splitting, I know, but .. it's hard to see what the effect would be if processing of annotations was allowed to happen concurrently. good point :) --kcc On Thu, Jan 28, 2010 at 10:32 AM, Bart Van Assche <bva...@ac...>wrote: > On Wed, Jan 27, 2010 at 6:09 PM, Julian Seward <js...@ac...> wrote: > > > > [ ... ] > > > > IOW we need to deal in the worst case with an arbitrary delay between > > a thread leaving the barrier proper and having its state updated at > > the B_DEPART request. > > > > Hence I am now thinking instead of the following annotations: > > > > B_INITIALISE(bar, int capacity) > > initialise; state capacity > > > > void* abstract_handle = B_ARRIVE(bar) > > arrive at barrier. obtain abstract handle (a VTS*, essentially) > from > > which we need to update when leaving > > > > B_DEPART(bar, abstract_handle) > > depart from barrier, updating our VTS using the one we were given > > by B_ARRIVE > > The above looks a lot better to me than instrumenting user-implemented > barriers via AH_BEFORE() and AH_AFTER(). While the latter approach is > sufficient for suppressing false positives triggered by a user barrier > implementation, these is a high risk that instrumenting > user-implemented barriers via AH_BEFORE() and AH_AFTER() will suppress > real races because these primitives introduce more inter-thread > ordering than barriers. The above proposal doesn't have that > disadvantage. > > By the way, the "abstract_handle" is unnecessary in my opinion. Since > B_DEPART() will always be invoked from the same thread as the > corresponding B_ARRIVE() call, a threading tool can figure out which > B_DEPART() corresponds to B_ARRIVE() call by keeping a limited amount > of state information. > > Bart. > |
|
From: Julian S. <js...@ac...> - 2010-01-28 09:47:51
|
On Thursday 28 January 2010, Konstantin Serebryany wrote: > I agree that the current annotations used to implement barrier support in > ThreadSanitizer *may* *in theory* hide some races. > But I would like to see an example where it really hides them. > And then, I would like to see a *realistic* example. :) I see what you are saying. But overall I would much prefer to have an annotation mechanism which can express exactly the set of inter-thread dependencies, without adding any unnecessary ones. It may be mostly of theoretical interest, but I think it is conceptually cleaner. From the perspective of debugging and verifying the checkers, it's going to be more difficult to reason about behaviour if the checkers are dealing with a mixture of required and "fake" inter-thread dependencies. > I agree that AH_BEFORE_OVERWRITE and A_FORGET might be needed in some > cases. (but again, I'd like to see a *realistic* test where the current > annotations hide a race) > But the problem with the annotations is that we already have too many of > them and the users are sometimes confused. Maybe we should review the set to see if any are unneccessary. > Also, are we talking here about implementing barrier support inside a tool > (helgrind, drd, tsan, etc) or about exporting these annotations to users so > that they can use it for their custom barriers? Mostly the latter. But once we can correctly annotate custom barriers, exactly the same annotations should be usable to annotation pthread_barrier_* functions. So, really, "both". J |
|
From: Konstantin S. <kon...@gm...> - 2010-01-28 15:17:31
|
On Thu, Jan 28, 2010 at 1:02 PM, Julian Seward <js...@ac...> wrote: > On Thursday 28 January 2010, Konstantin Serebryany wrote: > > I agree that the current annotations used to implement barrier support in > > ThreadSanitizer *may* *in theory* hide some races. > > But I would like to see an example where it really hides them. > > And then, I would like to see a *realistic* example. :) > > I see what you are saying. But overall I would much prefer to have > an annotation mechanism which can express exactly the set of inter-thread > dependencies, without adding any unnecessary ones. It may be mostly > of theoretical interest, but I think it is conceptually cleaner. From > the perspective of debugging and verifying the checkers, it's going to > be more difficult to reason about behaviour if the checkers are > dealing with a mixture of required and "fake" inter-thread dependencies. > Yeaaa. Still, I want a unit test were the current implementation hides a race. W/o a unittest, how do we test that the new implementation is correct (or at least better)? > > > I agree that AH_BEFORE_OVERWRITE and A_FORGET might be needed in some > > cases. (but again, I'd like to see a *realistic* test where the current > > annotations hide a race) > > But the problem with the annotations is that we already have too many of > > them and the users are sometimes confused. > > Maybe we should review the set to see if any are unneccessary. > There are two which are broken, I need to get rid of them. ANNOTATE_UNPUBLISH... is completely broken (bad idea) ANNOTATE_PUBLISH... is broken in case we are using literace-like sampling The rest are heavily used... > > > > Also, are we talking here about implementing barrier support inside a > tool > > (helgrind, drd, tsan, etc) or about exporting these annotations to users > so > > that they can use it for their custom barriers? > > Mostly the latter. But once we can correctly annotate custom barriers, > exactly the same annotations should be usable to annotation > pthread_barrier_* > functions. So, really, "both". > > J > |
|
From: Julian S. <js...@ac...> - 2010-01-28 15:33:55
|
> Yeaaa.
> Still, I want a unit test were the current implementation hides a race.
> W/o a unittest, how do we test that the new implementation is correct (or
> at least better)?
Fair enough.
Something like this
4 threads, call them A1 A2 B1 and B2
1 barrier B of size 2
1 shared location L
All 4 threads created
B1 and B2 sleep(1)
A1 (or A2) writes to L
A1 and A2 do pthread_barrier_wait(B)
A1 and A2 sleep(1)
now B1 and B2 wake up. They do pthread_barrier_wait(B), thereby
falsely acquiring dependencies on A1 and A2's previous use of B
B1 (or B2) writes to L. This is really a race, but not detected due
to the false dependencies.
All 4 threads pth_join back to waiting parent.
Does that make sense? I think it should demonstrate the problem.
> There are two which are broken, I need to get rid of them.
> ANNOTATE_UNPUBLISH... is completely broken (bad idea)
> ANNOTATE_PUBLISH... is broken in case we are using literace-like sampling
I'll delete them in helgrind.h then.
J
|
|
From: Julian S. <js...@ac...> - 2010-01-28 17:31:06
|
On Thursday 28 January 2010, Julian Seward wrote:
> > Yeaaa.
> > Still, I want a unit test were the current implementation hides a race.
> > W/o a unittest, how do we test that the new implementation is correct (or
> > at least better)?
>
> Fair enough.
>
> Something like this [...]
Demo program using libpthread is below.
Helgrind(trunk) says
==15851== Possible data race during write of size 4 at 0x601060 by thread #4
==15851== at 0x4007D8: actions_b1 (pthb_reuse_race.c:34)
==15851== by 0x4C29752: mythread_wrapper (hg_intercepts.c:202)
==15851== by 0x4E3403F: start_thread (in /lib64/libpthread-2.8.so)
==15851== by 0x511C08C: clone (in /lib64/libc-2.8.so)
==15851== This conflicts with a previous write of size 4 by thread #2
==15851== at 0x400812: actions_a1 (pthb_reuse_race.c:19)
==15851== by 0x4C29752: mythread_wrapper (hg_intercepts.c:202)
==15851== by 0x4E3403F: start_thread (in /lib64/libpthread-2.8.so)
==15851== by 0x511C08C: clone (in /lib64/libc-2.8.so)
DRD(trunk) doesn't say anything.
TSan also doesn't say anything.
J
#define _GNU_SOURCE
#include <assert.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
/* Shared location, accessed by A1 (or A2) and B1 (or B2) */
int L;
/* The barrier, size 2 */
pthread_barrier_t B;
// A1/A2: write L, then wait for barrier, then sleep
void* actions_a1 ( void* v ) {
L = 1;
pthread_barrier_wait(&B);
sleep(1);
return NULL;
}
void* actions_a2 ( void* v ) {
pthread_barrier_wait(&B);
sleep(1);
return NULL;
}
// B1/B2: sleep, wait for barrier, then write L
void* actions_b1 ( void* v ) {
sleep(1);
pthread_barrier_wait(&B);
L = 1;
return NULL;
}
void* actions_b2 ( void* v ) {
sleep(1);
pthread_barrier_wait(&B);
return NULL;
}
int main ( void )
{
pthread_t a1, a2, b1, b2;
pthread_barrier_init(&B, NULL, 2);
pthread_create(&a1, NULL, actions_a1, NULL);
pthread_create(&a2, NULL, actions_a2, NULL);
pthread_create(&b1, NULL, actions_b1, NULL);
pthread_create(&b2, NULL, actions_b2, NULL);
pthread_join(a1, NULL);
pthread_join(a2, NULL);
pthread_join(b1, NULL);
pthread_join(b2, NULL);
return 0;
}
|
|
From: Konstantin S. <kon...@gm...> - 2010-01-28 19:17:41
|
ahhh.
I again forgot that pthread_barrier is cyclic (resettable).
imho this is error prone and dull, but that's what we have.
I think we will need two annotations to fully support it:
// inserted before the actual barrier_init code
ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
// inserted before the actual barrier_wait code.
ANNOTATE_CYCLIC_BARRIER_WAIT(obj)
I'll give it a bite on Fri or Mon.
--kcc
On Thu, Jan 28, 2010 at 8:45 PM, Julian Seward <js...@ac...> wrote:
> On Thursday 28 January 2010, Julian Seward wrote:
> > > Yeaaa.
> > > Still, I want a unit test were the current implementation hides a race.
> > > W/o a unittest, how do we test that the new implementation is correct
> (or
> > > at least better)?
> >
> > Fair enough.
> >
> > Something like this [...]
>
> Demo program using libpthread is below.
>
> Helgrind(trunk) says
>
> ==15851== Possible data race during write of size 4 at 0x601060 by thread
> #4
> ==15851== at 0x4007D8: actions_b1 (pthb_reuse_race.c:34)
> ==15851== by 0x4C29752: mythread_wrapper (hg_intercepts.c:202)
> ==15851== by 0x4E3403F: start_thread (in /lib64/libpthread-2.8.so)
> ==15851== by 0x511C08C: clone (in /lib64/libc-2.8.so)
> ==15851== This conflicts with a previous write of size 4 by thread #2
> ==15851== at 0x400812: actions_a1 (pthb_reuse_race.c:19)
> ==15851== by 0x4C29752: mythread_wrapper (hg_intercepts.c:202)
> ==15851== by 0x4E3403F: start_thread (in /lib64/libpthread-2.8.so)
> ==15851== by 0x511C08C: clone (in /lib64/libc-2.8.so)
>
> DRD(trunk) doesn't say anything.
>
> TSan also doesn't say anything.
>
> J
>
>
>
> #define _GNU_SOURCE
>
> #include <assert.h>
> #include <pthread.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> /* Shared location, accessed by A1 (or A2) and B1 (or B2) */
> int L;
>
> /* The barrier, size 2 */
> pthread_barrier_t B;
>
>
> // A1/A2: write L, then wait for barrier, then sleep
> void* actions_a1 ( void* v ) {
> L = 1;
> pthread_barrier_wait(&B);
> sleep(1);
> return NULL;
> }
> void* actions_a2 ( void* v ) {
> pthread_barrier_wait(&B);
> sleep(1);
> return NULL;
> }
>
> // B1/B2: sleep, wait for barrier, then write L
> void* actions_b1 ( void* v ) {
> sleep(1);
> pthread_barrier_wait(&B);
> L = 1;
> return NULL;
> }
> void* actions_b2 ( void* v ) {
> sleep(1);
> pthread_barrier_wait(&B);
> return NULL;
> }
>
>
> int main ( void )
> {
> pthread_t a1, a2, b1, b2;
> pthread_barrier_init(&B, NULL, 2);
>
> pthread_create(&a1, NULL, actions_a1, NULL);
> pthread_create(&a2, NULL, actions_a2, NULL);
> pthread_create(&b1, NULL, actions_b1, NULL);
> pthread_create(&b2, NULL, actions_b2, NULL);
>
> pthread_join(a1, NULL);
> pthread_join(a2, NULL);
> pthread_join(b1, NULL);
> pthread_join(b2, NULL);
>
> return 0;
> }
>
|
|
From: Bart V. A. <bva...@ac...> - 2010-01-29 07:12:41
|
On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany <kon...@gm...> wrote: > I again forgot that pthread_barrier is cyclic (resettable). > imho this is error prone and dull, but that's what we have. > I think we will need two annotations to fully support it: > // inserted before the actual barrier_init code > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n) > // inserted before the actual barrier_wait code. > ANNOTATE_CYCLIC_BARRIER_WAIT(obj) Hello Konstantin, Why do feel that there is a need to tell a thread-checking tool on beforehand whether a barrier will be used only once or multiple times ? Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-01-29 07:15:32
|
On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche <bva...@ac...>wrote: > On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany > <kon...@gm...> wrote: > > I again forgot that pthread_barrier is cyclic (resettable). > > imho this is error prone and dull, but that's what we have. > > I think we will need two annotations to fully support it: > > // inserted before the actual barrier_init code > > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n) > > // inserted before the actual barrier_wait code. > > ANNOTATE_CYCLIC_BARRIER_WAIT(obj) > > Hello Konstantin, > > Why do feel that there is a need to tell a thread-checking tool on > beforehand whether a barrier will be used only once or multiple times > There is not such need. I think the name should contain 'CYCLIC' to make it clear that the annotation supports cyclic barriers. It will of course work with non-resettable barriers as well. --kcc > ? > > Bart. > |
|
From: Bart V. A. <bva...@ac...> - 2010-01-29 09:46:39
|
On Fri, Jan 29, 2010 at 8:15 AM, Konstantin Serebryany <kon...@gm...> wrote: > > On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche <bva...@ac...> > wrote: >> >> On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany >> <kon...@gm...> wrote: >> > I again forgot that pthread_barrier is cyclic (resettable). >> > imho this is error prone and dull, but that's what we have. >> > I think we will need two annotations to fully support it: >> > // inserted before the actual barrier_init code >> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n) >> > // inserted before the actual barrier_wait code. >> > ANNOTATE_CYCLIC_BARRIER_WAIT(obj) >> >> Hello Konstantin, >> >> Why do feel that there is a need to tell a thread-checking tool on >> beforehand whether a barrier will be used only once or multiple times > > There is not such need. > I think the name should contain 'CYCLIC' to make it clear that the > annotation supports cyclic barriers. > It will of course work with non-resettable barriers as well. As far as I know the term 'cyclic barrier' is only used in the context of the Java programming language and not in any document about POSIX barriers. So this terminology might be confusing for who's familiar with POSIX threads but not with Java. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-01-29 09:39:09
|
And of course, we need 3 annotations:
ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
ANNOTATE_CYCLIC_BARRIER_WAIT_BEFORE(obj)
ANNOTATE_CYCLIC_BARRIER_WAIT_AFTER(obj)
Here are the relevant bits of tsan code to support them:
------------------------------------------------------------------------------------------
// Support for Cyclic Barrier, e.g. pthread_barrier_t.
// We need to create (barrier_count-1)^2 h-b arcs between
// threads blocking on a barrier. We should not create any h-b arcs
// for two calls to barrier_wait if the barrier was reset between then.
struct CyclicBarrierInfo {
// The value given to barrier_init.
uint32_t barrier_count;
// How many times we may block on this barrier before resetting.
int32_t calls_before_reset;
};
// Maps the barrier pointer to CyclicBarrierInfo.
typedef hash_map<uintptr_t, CyclicBarrierInfo> CyclicBarrierMap;
CyclicBarrierInfo &GetCyclicBarrierInfo(uintptr_t barrier) {
if (cyclic_barrier_map_ == NULL) {
cyclic_barrier_map_ = new CyclicBarrierMap;
}
return (*cyclic_barrier_map_)[barrier];
}
void HandleBarrierInit(uintptr_t barrier, uint32_t n) {
CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
info.barrier_count = n;
info.calls_before_reset = 0;
}
void HandleBarrierWaitBefore(uintptr_t barrier) {
CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
CHECK(info.calls_before_reset >= 0);
if (info.calls_before_reset == 0) {
// We are blocking the first time after reset. Clear the VTS.
info.calls_before_reset = info.barrier_count;
Signaller &signaller = (*signaller_map_)[barrier];
VTS::Delete(signaller.vts);
signaller.vts = NULL;
}
info.calls_before_reset--;
// Signal to all threads that blocked on this barrier.
HandleSignal(barrier);
}
void HandleBarrierWaitAfter(uintptr_t barrier) {
HandleWait(barrier, 0, false);
}
------------------------------------------------------------------------------------------
Here is the report:
==5414== WARNING: Expected data race during write of size 4 at 0xC968EE0:
{{{
==5414== T7 (locks held: {}):
==5414== #0 PositiveTests_CyclicBarrierTest::b1()
/home/kcc/drt/trunk/unittest/posix_tests.cc:726
==5414== #1 MyThread::ThreadBody(MyThread*)
/home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
==5414== #2 ThreadSanitizerStartThread
/home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
==5414== Concurrent write(s) happened at (OR AFTER) these points:
==5414== T4 (locks held: {}):
==5414== #0 PositiveTests_CyclicBarrierTest::a1()
/home/kcc/drt/trunk/unittest/posix_tests.cc:712
==5414== #1 MyThread::ThreadBody(MyThread*)
/home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
==5414== #2 ThreadSanitizerStartThread
/home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
==5414== Address 0xC968EE0 is 0 bytes inside data symbol
"_ZN31PositiveTests_CyclicBarrierTest1LE"
==5414== Description: "real race, may be hidden by incorrect implementation
of barrier"
==5414== }}}
--kcc
On Fri, Jan 29, 2010 at 10:15 AM, Konstantin Serebryany <
kon...@gm...> wrote:
>
>
> On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche <bva...@ac...>wrote:
>
>> On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany
>> <kon...@gm...> wrote:
>> > I again forgot that pthread_barrier is cyclic (resettable).
>> > imho this is error prone and dull, but that's what we have.
>> > I think we will need two annotations to fully support it:
>> > // inserted before the actual barrier_init code
>> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
>> > // inserted before the actual barrier_wait code.
>> > ANNOTATE_CYCLIC_BARRIER_WAIT(obj)
>>
>> Hello Konstantin,
>>
>> Why do feel that there is a need to tell a thread-checking tool on
>> beforehand whether a barrier will be used only once or multiple times
>>
>
> There is not such need.
> I think the name should contain 'CYCLIC' to make it clear that the
> annotation supports cyclic barriers.
> It will of course work with non-resettable barriers as well.
>
> --kcc
>
>
>> ?
>>
>> Bart.
>>
>
>
|
|
From: Julian S. <js...@ac...> - 2010-01-29 09:48:25
|
On Friday 29 January 2010, Konstantin Serebryany wrote:
> And of course, we need 3 annotations:
>
> ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
> ANNOTATE_CYCLIC_BARRIER_WAIT_BEFORE(obj)
> ANNOTATE_CYCLIC_BARRIER_WAIT_AFTER(obj)
Ok, but .. couldn't we get rid of the "CYCLIC" bit? Barriers are
widely understood to be cyclic. Just seems like pointless redundancy.
Also, I would like to add
ANNOTATE_BARRIER_DESTROY(obj)
so that a tool can observe the entire lifetime of a barrier if
required. And so that the same annotations can be used to instrument
custom barriers and pthread_barriers.
J
>
> Here are the relevant bits of tsan code to support them:
> ---------------------------------------------------------------------------
>--------------- // Support for Cyclic Barrier, e.g. pthread_barrier_t.
> // We need to create (barrier_count-1)^2 h-b arcs between
> // threads blocking on a barrier. We should not create any h-b arcs
> // for two calls to barrier_wait if the barrier was reset between then.
> struct CyclicBarrierInfo {
> // The value given to barrier_init.
> uint32_t barrier_count;
> // How many times we may block on this barrier before resetting.
> int32_t calls_before_reset;
> };
> // Maps the barrier pointer to CyclicBarrierInfo.
> typedef hash_map<uintptr_t, CyclicBarrierInfo> CyclicBarrierMap;
>
> CyclicBarrierInfo &GetCyclicBarrierInfo(uintptr_t barrier) {
> if (cyclic_barrier_map_ == NULL) {
> cyclic_barrier_map_ = new CyclicBarrierMap;
> }
> return (*cyclic_barrier_map_)[barrier];
> }
>
> void HandleBarrierInit(uintptr_t barrier, uint32_t n) {
> CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
> info.barrier_count = n;
> info.calls_before_reset = 0;
> }
>
> void HandleBarrierWaitBefore(uintptr_t barrier) {
> CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
>
> CHECK(info.calls_before_reset >= 0);
> if (info.calls_before_reset == 0) {
> // We are blocking the first time after reset. Clear the VTS.
> info.calls_before_reset = info.barrier_count;
> Signaller &signaller = (*signaller_map_)[barrier];
> VTS::Delete(signaller.vts);
> signaller.vts = NULL;
> }
> info.calls_before_reset--;
> // Signal to all threads that blocked on this barrier.
> HandleSignal(barrier);
> }
>
> void HandleBarrierWaitAfter(uintptr_t barrier) {
> HandleWait(barrier, 0, false);
> }
> ---------------------------------------------------------------------------
>--------------- Here is the report:
> ==5414== WARNING: Expected data race during write of size 4 at 0xC968EE0:
> {{{
> ==5414== T7 (locks held: {}):
> ==5414== #0 PositiveTests_CyclicBarrierTest::b1()
> /home/kcc/drt/trunk/unittest/posix_tests.cc:726
> ==5414== #1 MyThread::ThreadBody(MyThread*)
> /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
> ==5414== #2 ThreadSanitizerStartThread
> /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
> ==5414== Concurrent write(s) happened at (OR AFTER) these points:
> ==5414== T4 (locks held: {}):
> ==5414== #0 PositiveTests_CyclicBarrierTest::a1()
> /home/kcc/drt/trunk/unittest/posix_tests.cc:712
> ==5414== #1 MyThread::ThreadBody(MyThread*)
> /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
> ==5414== #2 ThreadSanitizerStartThread
> /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
> ==5414== Address 0xC968EE0 is 0 bytes inside data symbol
> "_ZN31PositiveTests_CyclicBarrierTest1LE"
> ==5414== Description: "real race, may be hidden by incorrect
> implementation of barrier"
> ==5414== }}}
>
>
>
> --kcc
>
>
>
>
>
>
>
> On Fri, Jan 29, 2010 at 10:15 AM, Konstantin Serebryany <
>
> kon...@gm...> wrote:
> > On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche
<bva...@ac...>wrote:
> >> On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany
> >>
> >> <kon...@gm...> wrote:
> >> > I again forgot that pthread_barrier is cyclic (resettable).
> >> > imho this is error prone and dull, but that's what we have.
> >> > I think we will need two annotations to fully support it:
> >> > // inserted before the actual barrier_init code
> >> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
> >> > // inserted before the actual barrier_wait code.
> >> > ANNOTATE_CYCLIC_BARRIER_WAIT(obj)
> >>
> >> Hello Konstantin,
> >>
> >> Why do feel that there is a need to tell a thread-checking tool on
> >> beforehand whether a barrier will be used only once or multiple times
> >
> > There is not such need.
> > I think the name should contain 'CYCLIC' to make it clear that the
> > annotation supports cyclic barriers.
> > It will of course work with non-resettable barriers as well.
> >
> > --kcc
> >
> >> ?
> >>
> >> Bart.
|
|
From: Konstantin S. <kon...@gm...> - 2010-01-29 09:52:11
|
On Fri, Jan 29, 2010 at 1:02 PM, Julian Seward <js...@ac...> wrote:
> On Friday 29 January 2010, Konstantin Serebryany wrote:
> > And of course, we need 3 annotations:
> >
> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
> > ANNOTATE_CYCLIC_BARRIER_WAIT_BEFORE(obj)
> > ANNOTATE_CYCLIC_BARRIER_WAIT_AFTER(obj)
>
> Ok, but .. couldn't we get rid of the "CYCLIC" bit? Barriers are
> widely understood to be cyclic. Just seems like pointless redundancy.
>
2:1
I give up. :)
>
> Also, I would like to add
>
> ANNOTATE_BARRIER_DESTROY(obj)
>
Yep.
Not that is reeeeeally badly required for the implementation, but nice for
consistency.
So, we end up with
ANNOTATE_BARRIER_INIT(obj, n)
ANNOTATE_BARRIER_WAIT_BEFORE(obj)
ANNOTATE_BARRIER_WAIT_AFTER(obj)
ANNOTATE_BARRIER_DESTROY(obj)
--kcc
>
> so that a tool can observe the entire lifetime of a barrier if
> required. And so that the same annotations can be used to instrument
> custom barriers and pthread_barriers.
>
> J
>
> >
> > Here are the relevant bits of tsan code to support them:
> >
> ---------------------------------------------------------------------------
> >--------------- // Support for Cyclic Barrier, e.g. pthread_barrier_t.
> > // We need to create (barrier_count-1)^2 h-b arcs between
> > // threads blocking on a barrier. We should not create any h-b arcs
> > // for two calls to barrier_wait if the barrier was reset between then.
> > struct CyclicBarrierInfo {
> > // The value given to barrier_init.
> > uint32_t barrier_count;
> > // How many times we may block on this barrier before resetting.
> > int32_t calls_before_reset;
> > };
> > // Maps the barrier pointer to CyclicBarrierInfo.
> > typedef hash_map<uintptr_t, CyclicBarrierInfo> CyclicBarrierMap;
> >
> > CyclicBarrierInfo &GetCyclicBarrierInfo(uintptr_t barrier) {
> > if (cyclic_barrier_map_ == NULL) {
> > cyclic_barrier_map_ = new CyclicBarrierMap;
> > }
> > return (*cyclic_barrier_map_)[barrier];
> > }
> >
> > void HandleBarrierInit(uintptr_t barrier, uint32_t n) {
> > CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
> > info.barrier_count = n;
> > info.calls_before_reset = 0;
> > }
> >
> > void HandleBarrierWaitBefore(uintptr_t barrier) {
> > CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier);
> >
> > CHECK(info.calls_before_reset >= 0);
> > if (info.calls_before_reset == 0) {
> > // We are blocking the first time after reset. Clear the VTS.
> > info.calls_before_reset = info.barrier_count;
> > Signaller &signaller = (*signaller_map_)[barrier];
> > VTS::Delete(signaller.vts);
> > signaller.vts = NULL;
> > }
> > info.calls_before_reset--;
> > // Signal to all threads that blocked on this barrier.
> > HandleSignal(barrier);
> > }
> >
> > void HandleBarrierWaitAfter(uintptr_t barrier) {
> > HandleWait(barrier, 0, false);
> > }
> >
> ---------------------------------------------------------------------------
> >--------------- Here is the report:
> > ==5414== WARNING: Expected data race during write of size 4 at 0xC968EE0:
> > {{{
> > ==5414== T7 (locks held: {}):
> > ==5414== #0 PositiveTests_CyclicBarrierTest::b1()
> > /home/kcc/drt/trunk/unittest/posix_tests.cc:726
> > ==5414== #1 MyThread::ThreadBody(MyThread*)
> > /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
> > ==5414== #2 ThreadSanitizerStartThread
> > /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
> > ==5414== Concurrent write(s) happened at (OR AFTER) these points:
> > ==5414== T4 (locks held: {}):
> > ==5414== #0 PositiveTests_CyclicBarrierTest::a1()
> > /home/kcc/drt/trunk/unittest/posix_tests.cc:712
> > ==5414== #1 MyThread::ThreadBody(MyThread*)
> > /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328
> > ==5414== #2 ThreadSanitizerStartThread
> > /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535
> > ==5414== Address 0xC968EE0 is 0 bytes inside data symbol
> > "_ZN31PositiveTests_CyclicBarrierTest1LE"
> > ==5414== Description: "real race, may be hidden by incorrect
> > implementation of barrier"
> > ==5414== }}}
> >
> >
> >
> > --kcc
> >
> >
> >
> >
> >
> >
> >
> > On Fri, Jan 29, 2010 at 10:15 AM, Konstantin Serebryany <
> >
> > kon...@gm...> wrote:
> > > On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche
> <bva...@ac...>wrote:
> > >> On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany
> > >>
> > >> <kon...@gm...> wrote:
> > >> > I again forgot that pthread_barrier is cyclic (resettable).
> > >> > imho this is error prone and dull, but that's what we have.
> > >> > I think we will need two annotations to fully support it:
> > >> > // inserted before the actual barrier_init code
> > >> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n)
> > >> > // inserted before the actual barrier_wait code.
> > >> > ANNOTATE_CYCLIC_BARRIER_WAIT(obj)
> > >>
> > >> Hello Konstantin,
> > >>
> > >> Why do feel that there is a need to tell a thread-checking tool on
> > >> beforehand whether a barrier will be used only once or multiple times
> > >
> > > There is not such need.
> > > I think the name should contain 'CYCLIC' to make it clear that the
> > > annotation supports cyclic barriers.
> > > It will of course work with non-resettable barriers as well.
> > >
> > > --kcc
> > >
> > >> ?
> > >>
> > >> Bart.
>
>
>
|
|
From: Konstantin S. <kon...@gm...> - 2010-01-29 10:33:51
|
Please check here: http://codereview.appspot.com/196059/patch/1/3 On Fri, Jan 29, 2010 at 12:51 PM, Konstantin Serebryany < kon...@gm...> wrote: > > > On Fri, Jan 29, 2010 at 1:02 PM, Julian Seward <js...@ac...> wrote: > >> On Friday 29 January 2010, Konstantin Serebryany wrote: >> > And of course, we need 3 annotations: >> > >> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n) >> > ANNOTATE_CYCLIC_BARRIER_WAIT_BEFORE(obj) >> > ANNOTATE_CYCLIC_BARRIER_WAIT_AFTER(obj) >> >> Ok, but .. couldn't we get rid of the "CYCLIC" bit? Barriers are >> widely understood to be cyclic. Just seems like pointless redundancy. >> > 2:1 > I give up. :) > >> >> Also, I would like to add >> >> ANNOTATE_BARRIER_DESTROY(obj) >> > > Yep. > Not that is reeeeeally badly required for the implementation, but nice for > consistency. > So, we end up with > ANNOTATE_BARRIER_INIT(obj, n) > ANNOTATE_BARRIER_WAIT_BEFORE(obj) > ANNOTATE_BARRIER_WAIT_AFTER(obj) > ANNOTATE_BARRIER_DESTROY(obj) > > --kcc > > >> >> so that a tool can observe the entire lifetime of a barrier if >> required. And so that the same annotations can be used to instrument >> custom barriers and pthread_barriers. >> >> J >> >> > >> > Here are the relevant bits of tsan code to support them: >> > >> --------------------------------------------------------------------------- >> >--------------- // Support for Cyclic Barrier, e.g. pthread_barrier_t. >> > // We need to create (barrier_count-1)^2 h-b arcs between >> > // threads blocking on a barrier. We should not create any h-b arcs >> > // for two calls to barrier_wait if the barrier was reset between >> then. >> > struct CyclicBarrierInfo { >> > // The value given to barrier_init. >> > uint32_t barrier_count; >> > // How many times we may block on this barrier before resetting. >> > int32_t calls_before_reset; >> > }; >> > // Maps the barrier pointer to CyclicBarrierInfo. >> > typedef hash_map<uintptr_t, CyclicBarrierInfo> CyclicBarrierMap; >> > >> > CyclicBarrierInfo &GetCyclicBarrierInfo(uintptr_t barrier) { >> > if (cyclic_barrier_map_ == NULL) { >> > cyclic_barrier_map_ = new CyclicBarrierMap; >> > } >> > return (*cyclic_barrier_map_)[barrier]; >> > } >> > >> > void HandleBarrierInit(uintptr_t barrier, uint32_t n) { >> > CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier); >> > info.barrier_count = n; >> > info.calls_before_reset = 0; >> > } >> > >> > void HandleBarrierWaitBefore(uintptr_t barrier) { >> > CyclicBarrierInfo &info = GetCyclicBarrierInfo(barrier); >> > >> > CHECK(info.calls_before_reset >= 0); >> > if (info.calls_before_reset == 0) { >> > // We are blocking the first time after reset. Clear the VTS. >> > info.calls_before_reset = info.barrier_count; >> > Signaller &signaller = (*signaller_map_)[barrier]; >> > VTS::Delete(signaller.vts); >> > signaller.vts = NULL; >> > } >> > info.calls_before_reset--; >> > // Signal to all threads that blocked on this barrier. >> > HandleSignal(barrier); >> > } >> > >> > void HandleBarrierWaitAfter(uintptr_t barrier) { >> > HandleWait(barrier, 0, false); >> > } >> > >> --------------------------------------------------------------------------- >> >--------------- Here is the report: >> > ==5414== WARNING: Expected data race during write of size 4 at >> 0xC968EE0: >> > {{{ >> > ==5414== T7 (locks held: {}): >> > ==5414== #0 PositiveTests_CyclicBarrierTest::b1() >> > /home/kcc/drt/trunk/unittest/posix_tests.cc:726 >> > ==5414== #1 MyThread::ThreadBody(MyThread*) >> > /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328 >> > ==5414== #2 ThreadSanitizerStartThread >> > /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535 >> > ==5414== Concurrent write(s) happened at (OR AFTER) these points: >> > ==5414== T4 (locks held: {}): >> > ==5414== #0 PositiveTests_CyclicBarrierTest::a1() >> > /home/kcc/drt/trunk/unittest/posix_tests.cc:712 >> > ==5414== #1 MyThread::ThreadBody(MyThread*) >> > /home/kcc/drt/trunk/unittest/thread_wrappers_pthread.h:328 >> > ==5414== #2 ThreadSanitizerStartThread >> > /home/kcc/drt/trunk/tsan/ts_valgrind_intercepts.c:535 >> > ==5414== Address 0xC968EE0 is 0 bytes inside data symbol >> > "_ZN31PositiveTests_CyclicBarrierTest1LE" >> > ==5414== Description: "real race, may be hidden by incorrect >> > implementation of barrier" >> > ==5414== }}} >> > >> > >> > >> > --kcc >> > >> > >> > >> > >> > >> > >> > >> > On Fri, Jan 29, 2010 at 10:15 AM, Konstantin Serebryany < >> > >> > kon...@gm...> wrote: >> > > On Fri, Jan 29, 2010 at 10:12 AM, Bart Van Assche >> <bva...@ac...>wrote: >> > >> On Thu, Jan 28, 2010 at 8:17 PM, Konstantin Serebryany >> > >> >> > >> <kon...@gm...> wrote: >> > >> > I again forgot that pthread_barrier is cyclic (resettable). >> > >> > imho this is error prone and dull, but that's what we have. >> > >> > I think we will need two annotations to fully support it: >> > >> > // inserted before the actual barrier_init code >> > >> > ANNOTATE_CYCLIC_BARRIER_INIT(obj, n) >> > >> > // inserted before the actual barrier_wait code. >> > >> > ANNOTATE_CYCLIC_BARRIER_WAIT(obj) >> > >> >> > >> Hello Konstantin, >> > >> >> > >> Why do feel that there is a need to tell a thread-checking tool on >> > >> beforehand whether a barrier will be used only once or multiple times >> > > >> > > There is not such need. >> > > I think the name should contain 'CYCLIC' to make it clear that the >> > > annotation supports cyclic barriers. >> > > It will of course work with non-resettable barriers as well. >> > > >> > > --kcc >> > > >> > >> ? >> > >> >> > >> Bart. >> >> >> > |
|
From: Bart V. A. <bva...@ac...> - 2010-01-29 10:39:52
|
On Fri, Jan 29, 2010 at 11:33 AM, Konstantin Serebryany <kon...@gm...> wrote: > Please check here: http://codereview.appspot.com/196059/patch/1/3 Looks good to me. The only remarks I have are: * A nit pick: personally I would call the second argument of ANNOTATE_BARRIER_INIT() "count" instead of "counter". * Some barrier implementations (e.g. the one in libgomp) allow barrier reinitialization while others (e.g. POSIX threads) do not allow this. If we want threading tools to be able to complain about barrier reinitialization for barrier types for which this is not allowed we need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool whether or not reinitialization is allowed. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-01-29 10:49:23
|
On Fri, Jan 29, 2010 at 1:39 PM, Bart Van Assche <bva...@ac...> wrote: > On Fri, Jan 29, 2010 at 11:33 AM, Konstantin Serebryany > <kon...@gm...> wrote: > > Please check here: http://codereview.appspot.com/196059/patch/1/3 > > Looks good to me. The only remarks I have are: > * A nit pick: personally I would call the second argument of > ANNOTATE_BARRIER_INIT() "count" instead of "counter". > fixed > * Some barrier implementations (e.g. the one in libgomp) allow barrier > reinitialization while others (e.g. POSIX threads) do not allow this. > If we want threading tools to be able to complain about barrier > reinitialization for barrier types for which this is not allowed we > need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool > whether or not reinitialization is allowed. > hm. maybe. dunno. Usually, such checking is hacked into the implementation of the synchronization utlility itself... No? > > Bart. > |
|
From: Julian S. <js...@ac...> - 2010-01-29 10:58:46
|
On Friday 29 January 2010, Konstantin Serebryany wrote: > On Fri, Jan 29, 2010 at 1:39 PM, Bart Van Assche <bva...@ac...> wrote: > > On Fri, Jan 29, 2010 at 11:33 AM, Konstantin Serebryany > > > > <kon...@gm...> wrote: > > > Please check here: http://codereview.appspot.com/196059/patch/1/3 > > > > Looks good to me. The only remarks I have are: > > * A nit pick: personally I would call the second argument of > > ANNOTATE_BARRIER_INIT() "count" instead of "counter". > > fixed > > > * Some barrier implementations (e.g. the one in libgomp) allow barrier > > reinitialization while others (e.g. POSIX threads) do not allow this. > > If we want threading tools to be able to complain about barrier > > reinitialization for barrier types for which this is not allowed we > > need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool > > whether or not reinitialization is allowed. > > hm. maybe. dunno. > Usually, such checking is hacked into the implementation of the > synchronization utlility itself... No? Better to keep implementation and specification separate. Since the may/may-not-be-reinitialised property is something a tool might want to check, we need a way to tell it whether the barrier may be reinitialised. J |
|
From: Julian S. <js...@ac...> - 2010-01-29 10:55:48
|
> * Some barrier implementations (e.g. the one in libgomp) allow barrier > reinitialization while others (e.g. POSIX threads) do not allow this. > If we want threading tools to be able to complain about barrier > reinitialization for barrier types for which this is not allowed we > need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool > whether or not reinitialization is allowed. Yes, +1 for that. Can you also do the other changes we discussed, so that the different annotation sets can stay in sync? * get rid of ANNOTATE_UNPUBLISH and ANNOTATE_PUBLISH * rename ANNOTATE_HAPPENS_BEFORE(obj) to ANNOTATE_HAPPENS_BEFORE_ACCUMULATE(obj) maybe add a #define for backwards compatibility * add ANNOTATE_HAPPENS_BEFORE_OVERWRITE(obj) * add ANNOTATE_HAPPENS_BEFORE_FORGET(obj) J |
|
From: Bart V. A. <bva...@ac...> - 2010-01-29 15:49:16
|
On Fri, Jan 29, 2010 at 12:09 PM, Julian Seward <js...@ac...> wrote: > > > * Some barrier implementations (e.g. the one in libgomp) allow barrier > > reinitialization while others (e.g. POSIX threads) do not allow this. > > If we want threading tools to be able to complain about barrier > > reinitialization for barrier types for which this is not allowed we > > need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool > > whether or not reinitialization is allowed. > > Yes, +1 for that. > > Can you also do the other changes we discussed, so that the different > annotation sets can stay in sync? > > * get rid of ANNOTATE_UNPUBLISH and ANNOTATE_PUBLISH > > * rename ANNOTATE_HAPPENS_BEFORE(obj) > to ANNOTATE_HAPPENS_BEFORE_ACCUMULATE(obj) > maybe add a #define for backwards compatibility > > * add ANNOTATE_HAPPENS_BEFORE_OVERWRITE(obj) > > * add ANNOTATE_HAPPENS_BEFORE_FORGET(obj) > Hello Julian, Is the above question intended for me or for Konstantin ? Bart. |
|
From: Bart V. A. <bva...@ac...> - 2010-01-29 10:57:02
|
On Fri, Jan 29, 2010 at 11:48 AM, Konstantin Serebryany <kon...@gm...> wrote: > > On Fri, Jan 29, 2010 at 1:39 PM, Bart Van Assche <bva...@ac...> wrote: >> >> On Fri, Jan 29, 2010 at 11:33 AM, Konstantin Serebryany >> <kon...@gm...> wrote: >> > Please check here: http://codereview.appspot.com/196059/patch/1/3 >> >> Looks good to me. The only remarks I have are: >> * A nit pick: personally I would call the second argument of >> ANNOTATE_BARRIER_INIT() "count" instead of "counter". > > fixed Thanks. >> * Some barrier implementations (e.g. the one in libgomp) allow barrier >> reinitialization while others (e.g. POSIX threads) do not allow this. >> If we want threading tools to be able to complain about barrier >> reinitialization for barrier types for which this is not allowed we >> need a third argument for ANNOTATE_BARRIER_INIT() that tells the tool >> whether or not reinitialization is allowed. > > hm. maybe. dunno. > Usually, such checking is hacked into the implementation of the > synchronization utlility itself... No? The POSIX spec is ambiguous about reinitialization. Two quotes from http://www.opengroup.org/onlinepubs/000095399/functions/pthread_barrier_init.html: * The results are undefined if pthread_barrier_init() is called specifying an already initialized barrier. * [EBUSY] The implementation has detected an attempt to reinitialize a barrier while it is in use (for example, while being used in a pthread_barrier_wait() call) by another thread. Or: while it is specified that reinitializing an already initialized barrier yields undefined results, it is not specified that pthread_barrier_init() must return an error code when reinitializing a barrier that is not in use. So the only sure way to catch this kind of POSIX violations is to let the thread checking tool verify this. And a tool can only verify this when it has been informed about whether or not barrier reinitialization is allowed. Bart. |