|
From: Konstantin S. <kon...@gm...> - 2009-06-26 13:06:21
|
Just FYI. We've added dynamic annotations to Chromium: http://src.chromium.org/viewvc/chrome?view=rev&revision=19353 In Chromium there are few classes/functions that use atomic instructions (e.g. Singleton, atomic refcount), so that w/o the annotations every race detector will have false positives. I wonder if other large opensource projects (e.g. Firefox, OpenOffice, etc) use atomics for synchronization. Any experience detecting races there? --kcc On Thu, Jun 25, 2009 at 2:18 PM, Konstantin Serebryany<kon...@gm...> wrote: > On Thu, Jun 25, 2009 at 1:29 PM, Bart Van > Assche<bar...@gm...> wrote: >> 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. > > We can do this with aliases, no? > >> 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). > > You mean, the pthread_spin_lock()? > It should be supported directly, not annotated. > There is a problem however: pthread_spin_unlock and pthread_spin_init > are the same symbol in some libc implementations. That's another > story... > > --kcc >> >> Bart. >> > |