Re: [Libunwind-devel] [patch] Fix for fast unwind and tcmalloc not playing well together.

[Paul Pluzhnikov]

On Thu, Oct 20, 2011 at 8:18 AM, Arun Sharma <address@hidden> wrote:

> Paul: The reason why libpthread loops over the keys multiple times
> seems to be related to
>
> "A destructor function might, however, re-associate non-NULL values to
> that key or some other key"
>
> I'm guessing that most destructors don't do this, which is why your
> solution works.

Correct.

> But if we get keys with crazy destructors which call
> pthread_setspecific(), could we be in trouble again on the last
> iteration?

Hence "tls_cache = NULL". If some other destructor delays the same way as
we do (rare) and attempts to record a stack trace (probably even rarer)
then we'll re-allocate thread_cache anew, and leak it.

> How about:
>
> static __thread bool trace_destroy_started = false;
>
> trace_cache_free():
>  trace_destroy_started = true
>
> trace_cache_get():
>  if trace_destroy_started: return NULL

That would also work; the disadvantage is that you'll fall back to slow
unwind for any subsequent free. If the other dtor deallocates a huge data
structure (e.g. 1GB tree), then you'll potentially execute a lot of slow
unwinds.

> I also worry a bit about libpthread changing
> PTHREAD_DESTRUCTOR_ITERATIONS underneath us.

The decrease of PTHREAD_DESTRUCTOR_ITERATIONS on a given system seems very
unlikely: you'll have to rebuild all binaries; not just libunwind, or they
will all leak (if they delay destruction).

OTOH, the increase is "mostly harmless" -- we'll free cache slightly
earlier than we could have.

I propose combined approach: delay until PTHREAD_DESTRUCTOR_ITERATIONS,
then set "cache_destroyed" boolean so we don't re-allocate (and don't leak).

Re-tested on Linux/x86_64 (Ubuntu 10.04); no new failures.

[This patch also cleans up 'return 0;' vs. 'return NULL;']

Thanks,
-- 
Paul Pluzhnikov

libunwind-delay-dtor-20111020.txt
Description: Text document