Re: [Libunwind-devel] unw_set_cache_size and per-thread caches

[Bert Wesarg]

Milian,

On Fri, May 4, 2018 at 10:21 PM, Milian Wolff <address@hidden> wrote:
> Hey Bert, others.
>
> I notice that unw_set_cache_size is disabled / always fails when libunwind is
> compiled with --enable-per-thread-cache:
>
>   /* Currently not supported for per-thread cache due to memory leak */
>   /* A pthread-key destructor would work, but is not signal safe */
> #if defined(HAVE___THREAD) && HAVE___THREAD
>   return -1;
> #endif
>
> Can you, or someone else, give me some more information on the reasoning here?
>
> I'm especially surprised, since I personally would never consider
> unw_set_cache_size to be called from a signal. I am probably misunderstanding
> something here?

its not about /setting/ the cache size, its about thread termination.
Changing the cache size allocates memory for the cache (the default
cache size is inside the cache variable itself. Thus this extra
allocated memory needs to be released if the thread terminates. And
the suggestion from Dave was to use a pthread-key destructor.

HTH,
Bert

>
> Actually, I would even go as far as saying that the cache size can and should
> only be configured once before it was ever used. So flushing the caches
> shouldn't even be required... Could we maybe at least support this use-case in
> the HAVE___THREAD scenario? I.e. something like the following:
>
> diff --git a/src/mi/Gset_cache_size.c b/src/mi/Gset_cache_size.c
> index 07b282e2..f1f97eea 100644
> --- a/src/mi/Gset_cache_size.c
> +++ b/src/mi/Gset_cache_size.c
> @@ -38,12 +38,6 @@ unw_set_cache_size (unw_addr_space_t as, size_t size, int
> flag)
>    if (flag != 0)
>      return -1;
>
> -  /* Currently not supported for per-thread cache due to memory leak */
> -  /* A pthread-key destructor would work, but is not signal safe */
> -#if defined(HAVE___THREAD) && HAVE___THREAD
> -  return -1;
> -#endif
> -
>    /* Round up to next power of two, slowly but portably */
>    while(power < size)
>      {
> @@ -58,6 +52,13 @@ unw_set_cache_size (unw_addr_space_t as, size_t size, int
> flag)
>    if (log_size == as->global_cache.log_size)
>      return 0;   /* no change */
>
> +  /* Changing cache size after it was used once is not supported for per-
> thread cache due to memory leak */
> +  /* A pthread-key destructor would work, but is not signal safe */
> +#if defined(HAVE___THREAD) && HAVE___THREAD
> +  if (as->global_cache.log_size)
> +    return -1;
> +#endif
> +
>    as->global_cache.log_size = log_size;
>  #endif
>
> If that is acceptable, then I'll push this up for review on Github.
>
> Cheers
> --
> Milian Wolff
> address@hidden
> http://milianw.de