|
From: Julian S. <js...@ac...> - 2010-07-02 07:02:54
|
> I don't think we need to use ANONTATE_* 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. --------------- 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. J |