On Thu, Oct 20, 2011 at 8:18 AM, Arun Sharma <[email protected]> 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
diff --git a/src/x86_64/Gtrace.c b/src/x86_64/Gtrace.c
index 4a5b583..e0592c3 100644
--- a/src/x86_64/Gtrace.c
+++ b/src/x86_64/Gtrace.c
@@ -25,6 +25,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
#include "ucontext_i.h"
#include <signal.h>
+#include <limits.h>
#pragma weak pthread_once
#pragma weak pthread_key_create
@@ -45,6 +46,8 @@ typedef struct
unw_tdep_frame_t *frames;
size_t log_size;
size_t used;
+ size_t dtor_count; /* Counts how many times our destructor has already
+ been called. */
} unw_trace_cache_t;
static const unw_tdep_frame_t empty_frame = { 0, UNW_X86_64_FRAME_OTHER, -1,
-1, 0, -1, -1 };
@@ -52,14 +55,26 @@ static pthread_mutex_t trace_init_lock =
PTHREAD_MUTEX_INITIALIZER;
static pthread_once_t trace_cache_once = PTHREAD_ONCE_INIT;
static pthread_key_t trace_cache_key;
static struct mempool trace_cache_pool;
+static __thread unw_trace_cache_t *tls_cache;
+static __thread int tls_cache_destructed;
/* Free memory for a thread's trace cache. */
static void
trace_cache_free (void *arg)
{
unw_trace_cache_t *cache = arg;
+ if (++cache->dtor_count < PTHREAD_DESTRUCTOR_ITERATIONS)
+ {
+ /* Not yet our turn to get destroyed. Re-install ourselves into the key. */
+ pthread_setspecific(trace_cache_key, cache);
+ Debug(5, "delayed freeing cache %p (%llx to go)\n", cache,
+ PTHREAD_DESTRUCTOR_ITERATIONS - cache->dtor_count);
+ return;
+ }
munmap (cache->frames, (1u << cache->log_size) * sizeof(unw_tdep_frame_t));
mempool_free (&trace_cache_pool, cache);
+ tls_cache = NULL;
+ tls_cache_destructed = 1;
Debug(5, "freed cache %p\n", cache);
}
@@ -93,21 +108,32 @@ trace_cache_create (void)
{
unw_trace_cache_t *cache;
+ if (tls_cache_destructed)
+ {
+ /* The current thread is in the process of exiting. Don't recreate
+ cache, as we wouldn't have another chance to free it. */
+ Debug(5, "refusing to reallocate cache: "
+ "thread-locals are being deallocated\n");
+ return NULL;
+ }
+
if (! (cache = mempool_alloc(&trace_cache_pool)))
{
Debug(5, "failed to allocate cache\n");
- return 0;
+ return NULL;
}
if (! (cache->frames = trace_cache_buckets(1u << HASH_MIN_BITS)))
{
Debug(5, "failed to allocate buckets\n");
mempool_free(&trace_cache_pool, cache);
- return 0;
+ return NULL;
}
cache->log_size = HASH_MIN_BITS;
cache->used = 0;
+ cache->dtor_count = 0;
+ tls_cache_destructed = 0; /* Paranoia: should already be 0. */
Debug(5, "allocated cache %p\n", cache);
return cache;
}
@@ -135,8 +161,6 @@ trace_cache_expand (unw_trace_cache_t *cache)
return 0;
}
-static __thread unw_trace_cache_t *tls_cache;
-
/* Get the frame cache for the current thread. Create it if there is none. */
static unw_trace_cache_t *
trace_cache_get (void)
@@ -285,7 +309,7 @@ trace_lookup (unw_cursor_t *cursor,
if (unlikely(addr || cache->used >= cache_size / 2))
{
if (unlikely(trace_cache_expand (cache) < 0))
- return 0;
+ return NULL;
cache_size = 1u << cache->log_size;
slot = ((rip * 0x9e3779b97f4a7c16) >> 43) & (cache_size-1);
_______________________________________________
Libunwind-devel mailing list
[email protected]
https://lists.nongnu.org/mailman/listinfo/libunwind-devel
