|
From: Bart V. A. <bar...@gm...> - 2009-06-01 16:19:45
|
On Mon, Jun 1, 2009 at 10:28 AM, Konstantin Serebryany <kon...@gm...> wrote: > On Sun, May 31, 2009 at 11:14 PM, Bart Van Assche > <bar...@gm...> wrote: >> On Sat, May 30, 2009 at 9:12 PM, Bart Van Assche >> <bar...@gm...> wrote: >>> On Fri, May 29, 2009 at 12:58 PM, Konstantin Serebryany >>> <kon...@gm...> wrote: >>>> Do you plan to support annotations (aka client requests) in Helgrind >>>> and DRD in a compatible way (and possibly, in a way compatible with >>>> ThreadSanitizer)? >>>> Something like http://code.google.com/p/google-perftools/source/browse/trunk/src/base/dynamic_annotations.h, >>>> or completely different. >>>> Our experience shows that even a pure-happens-before race detector is >>>> completely useless w/o annotations if your code has lock-less >>>> synchronization and hundreds of benign races. >>> >>> Another remark: I suggest to remove the macro's >>> ANNOTATE_CONDVAR_WAIT() and ANNOTATE_CONDVAR_SIGNAL() but to keep >>> their aliases ANNOTATE_HAPPENS_BEFORE() and ANNOTATE_HAPPENS_AFTER(). >>> The names of the first two macro's are really confusing: these two >>> macro's are a.o. used to annotate ordering constraints between >>> mutexes(!) in racecheck_unittest.cc. >> >> Please ignore the above comment -- I was misled by the statement >> ANNOTATE_CONDVAR_SIGNAL(&mu) in racecheck_unittest.cc. By this time I >> figured out that &mu is not the address of a mutex but the address of >> a condition variable. > > The parameter of ANNOTATE_CONDVAR_ is any pointer. > Since I introduced the HAPPENS_BEFORE/AFTER aliases, CONDVAR > annotations are supposed to be used only with cond vars (which is only > required in hybrid mode). > The aliases are there just to avoid confusion. IMHO it would be an improvement if it would be specified explicitly in the header file dynamic_annotations.h that the ANNOTATE_CONDVAR_* macro's should be used only with condition variables (pthread_condvar_t*) and that the ANNOTATE_HAPPENS_* macro's should be used only with other objects than POSIX synchronization objects. That would allow pure happens-before data race detectors to ignore the ANNOTATE_CONDVAR_* annotations and only handle the ANNOTATE_HAPPENS_* annotations. Can you please also update regression tests 30 and 31 ? These tests currently use the ANNOTATE_CONDVAR_* macro's while according to what you wrote these should use the ANNOTATE_HAPPENS_* macro's. One additional remark regarding the ANNOTATE_HAPPENS_* macro's: a data race detection tool has to allocate some memory to keep track of the inter-thread ordering imposed by these annotations. Since ANNOTATE_HAPPENS_AFTER may be invoked multiple times with the same argument, a tool cannot know when it can free the memory allocated to implement such an annotation. I have added the ANNOTATE_HAPPENS_AFTER_DONE() annotation in drd.h for this purpose. Bart. |