|
From: Bart V. A. <bar...@gm...> - 2009-06-25 09:29:33
|
On Thu, Jun 25, 2009 at 11:15 AM, Konstantin Serebryany <kon...@gm...> wrote: > > On Wed, Jun 24, 2009 at 11:11 PM, Bart Van > Assche<bar...@gm...> wrote: > > On Fri, May 29, 2009 at 12:58 PM, Konstantin > > Serebryany<kon...@gm...> wrote: > >> [changed subject] > >> > >> Julian, Bart, > >> > >> 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. > > > > My last set of remarks on the annotations defined in dynamic_annotations.h: > > * Currently there are two macro's defined for annotating acquiring and > > releasing an rwlock, namely ANNOTATE_RWLOCK_ACQUIRED(lock, is_w) > > ANNOTATE_RWLOCK_RELEASED(lock, is_w). The second argument, is_w, is a > > boolean. Having to specify 1 for a writer lock and 0 for a reader lock > > makes readability of the annotations subobtimal. Has it already been > > considered to replace these two macro's by e.g. the following four > > macro's: ANNOTATE_ACQUIRED_READERLOCK(lock), > > ANNOTATE_ACQUIRED_WRITERLOCK(lock), ANNOTATE_RELEASED_READERLOCK(lock) > > and ANNOTATE_RELEASED_WRITERLOCK(lock) ? > > Instead of replacing, we can create aliases, e.g. > #define ANNOTATE_READERLOCK_ACQUIRED(lock) ANNOTATE_RWLOCK_ACQUIRED(lock, 0) That's also fine for me. > > * The source file thread_wrappers_pthread.h makes it clear that the > > ANNOTATE_RWLOCK_* macro's are used both for annotating reader-writer > > locks and for annotating spinlocks. > ... or any other kind of mutex lock. > > > It might be a good idea to define > > separate annotations for reader-writer locks and for spinlocks. > > Why? I don't really see a difference from a race detector's point of view. >From a data race detector's point of view this indeed doesn't make a big difference. But defining separate annotations for spinlocks makes the intention more clear in the annotated source code -- seeing ANNOTATE_RWLOCK_...() around spinlocks looks a little bit strange to me. And defining separate annotations would also slightly simplify the implementation of the rwlock annotations in data race detectors that already support spinlocks (currently only DRD). Bart. |