|
From: Konstantin S. <kon...@gm...> - 2009-06-02 07:37:24
|
On Mon, Jun 1, 2009 at 8:19 PM, Bart Van Assche <bar...@gm...> wrote: > 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. Hm. Dunno. Maybe. BTW, please don't forget that there are other synchronization primitives in the world, not just POSIX objects. > > 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. Done, thanks! > > 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. True. > 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. Grr. I would avoid doing this. Once the program frees the memory which was used as an argument to ANNOTATE_HAPPENS_*, the detector may release the resources. (ThreadSanitizer doesn't do this now. I haven't seen this as a problem, but maybe it is...) > > Bart. > |