|
From: Konstantin S. <kon...@gm...> - 2010-07-05 09:32:55
|
+da...@go... Julian, Bart, I've prepared a (draft) message to libstdc++. Please review it. You are welcome to edit it directly too. http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting --kcc On Fri, Jul 2, 2010 at 9:10 PM, Konstantin Serebryany <kon...@gm...> wrote: > On Fri, Jul 2, 2010 at 2:19 PM, Bart Van Assche <bva...@ac...> wrote: >> On Fri, Jul 2, 2010 at 9:03 AM, Julian Seward <js...@ac...> wrote: >>> >>> > I don't think we need to use ANNOTATE_* in standard libraries. >>> > Instead, I want to ask the library writers to arrange their code to >>> > make it easier to intercept by valgrind tools. >>> > In particular, for atomic ref counting I want to ask them to have a new >>> > function which simply means "atomically decrement reference count and >>> > return whether count is zero". That function could itself simply call >>> > __exchange_and_add_dispatch or whatever. >>> >>> Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc) >>> and returns the new T. Synthesising "is new count == 0" into a 32/64-bit >>> value is expensive -- requires "testl ; set ; movzbl" -- better simply >>> to return the new value, which we have for free on LL/SC based platforms >>> (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded >>> using "load, sub, cmpxchg, check-the-cmpxchg succeeded". >>> >>> Why not just use the gcc builtin naming scheme for atomic operations? >>> It's uniform across multiple archs, and makes it clear whether the >>> returned value is the value before or after the decrement. No need to >>> reinvent the wheel. >> >> Only instrumenting the atomic decrement is not sufficient IMHO. The >> atomic increment has to be instrumented too, such that it is possible >> for a data race detection tool to find out which happens-before arcs >> to insert. > > Instrumenting ref count decrement was never needed in my practice. > In my practice we needed to instrument a function RefCountIsOne() in > addition to RefCountIncrement(); > > bool RefCountIsOne() { > bool res = AtomicRead(&ref_count_) == 1; > if (res) { > ANNOTATE_HAPPENS_AFTER(&ref_count_); > } > return res; > } > ... > if (x.RefCountIsOne()) { > x.DoSomethingWithXWithoutLocksBecauseWeOwnXAndAreGoingToDeleteIt. > } > > >> >>> --------------- >>> >>> Kind-of related comment: >>> >>> One observation w.r.t. refcounting and C++ is, when the refcount makes >>> a 1 -> 0 transition then the object becomes exclusively owned by the >>> decrementing thread, and so it can run its destructor without any >>> locking. However, at least Helgrind does not understand that, and so >>> needs to be told the object is now exclusively owned by the calling >>> thread before running the destructor. >>> >>> In this case, the obvious thing to do seems like >>> >>> VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this)) >>> >>> but this is wrong in the presence of inheritance, when a class defines a >>> ::Release method, and is then subclassed, but the subclass doesn't provide >>> its own ::Release. Then "sizeof(*this)" will be too small, because it is >>> the size of the base class, not the runtime size. Also, in the presence of >>> multiple inheritance, there is no guarantee that "this" points to the >>> start of the object; we are only assured that it points either to the >>> start or somewhere within it. >>> >>> For this reason I recently added to Helgrind a new clreq, >>> VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as >>> VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points >>> into, if any, and it's OK if it's an interior pointer. >>> >>> Do TSan and DRD have anything similar? >>> >>> This isn't theoretical. I found it out the hard way when analysing >>> an ocean of false race reports when Helgrinding C++ code using atomic >>> refcounting and inheritance, earlier this year. >> >> This kind of issue can indeed arise when letting the client program >> communicate to the tool how many bytes will be freed. But why to >> invoke such a client request from the client code while the tool >> already has exact information about the number of bytes that will be >> freed ? I'm assuming here that each reference counted object is >> allocated via an individual memory request, and that no memory region >> contains two or more reference-counted objects that are freed >> individually. > > See my previous message. The deleted object could be a compound > structure, not just a sequence of bytes. > The is not the case for std::string, but could be easily the case for > some other C++ stuff from e.g. boost. > We have tons of such classes, but they all use a single ref-counting > utility annotated with HAPPENS_BEFORE/AFTER and so they are > race-detector friendly. > > > --kcc > >> >> Bart. >> > |