|
From: Bart V. A. <bva...@ac...> - 2010-07-02 10:19:27
|
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. > --------------- > > 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. Bart. |