|
From: Konstantin S. <kon...@gm...> - 2010-07-02 17:11:20
|
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.
>
|