[Libunwind-devel] unw_set_cache_size and per-thread caches

[Milian Wolff]

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?

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

[Bert Wesarg]

Milian,

Post by Milian Wolff
Hey Bert, others.
I notice that unw_set_cache_size is disabled / always fails when libunwind is
/* 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

Post by Milian Wolff
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
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
http://milianw.de

[Milian Wolff]

Post by Bert Wesarg
Milian,

Post by Milian Wolff
Hey Bert, others.
I notice that unw_set_cache_size is disabled / always fails when libunwind
is>
/* 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.

OK, and ASAN won't report a leak, since the memory is allocated via mmap. Now
I understand.

Since "signal safety" is the apparent reason for why this isn't supported,
similar to the other thread about tls_model("initial-exec"), I wonder:

Is libunwind ever signal safe when it's compiled with per-thread-caches? If
that is not the case, we could just add the pthread-key destructor.

If on the other hand, __thread is actually (currently) thread safe, then I
propose we add another configuration option to libunwind, which will basically
remove the signal safety guarantees and enable us to remove the tls_model and
set custom cache sizes.

Cheers