|
From: Konstantin S. <kon...@gm...> - 2010-07-02 17:04:12
|
On Fri, Jul 2, 2010 at 11:03 AM, Julian Seward <js...@ac...> wrote:
>
>> 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.
Ok.
> 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?
>From their naming it is not obvious that the functions will not be
used for something other that ref counting (they probably will).
> 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.
The only reliable way of annotating ref counting known to me is
ANONTATE_HAPPENS_BEFORE(&ref_count_);
if (AtomicRefcountDecrement(&ref_count_) == 0) {
ANONTATE_HAPPENS_AFTER(&ref_count_);
delete this;
}
Not that 'delete this' is not just a memory deallocation (free), but
also any amount of other stuff.
>
> 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.
This is not enough, imo.
When you make 'delete this', 'this' is not necessary a simple object
like string.
It could be a complicate class like e.g. a tree, which means you will
end up with lots of calls to free() and other computations.
>
> Do TSan and DRD have anything similar?
As I said, we need HAPPENS_BEFORE/AFTER.
>
> 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.
Aha! So, you share my pain! :)
--kcc
>
> J
>
|