Re: [lttng-dev] [PATCH lttng-ust] Fix: destructors do not run on probe provider dlclose

[Mathieu Desnoyers]

- On Feb 21, 2018, at 5:10 PM, Francis Deslauriers 
francis.deslauri...@efficios.com wrote:

> Calling dlclose on a probe provider library does not unregister the
> probes from the callsites as the destructors are not executed.

please specify that this is only true for the first shared object loading
the __tracepoints__disable_destructors into the symbol table. Otherwise,
the sentence above is unclear, and we can be led to think that the destructor
is _never_ executed.


> 
> The __tracepoints__disable_destructors weak symbol was exposed by probe

please use current tense: "is exposed"

> providers, liblttng-ust.so and liblttng-ust-tracepoint.so libraries. If
> a probe provider was loaded first into the address space, its definition

"is loaded"

> would be binded to the symbol. All the subsequent libraries using the

"is bound"

> symbol would use the existing definition of the symbol. Thus creating a

", thus..."

> a situation where liblttng-ust.so or liblttng-ust-tracepoint.so would
> have a dependency on the probe provider library.

would have a dependency -> depend

> 
> This was preventing the dynamic loader from unloading the library as it

was preventing -> prevents

> was still in use by other libraries. Because of this, the execution of

is still

> its destructors and the unregistration of the probes was postponed.

is postponed

> 
> To overcome this issue, we no longer expose this symbol in the
> tracepoint.h file to remove the explicit dependency of the probe
> provider on the symbol. We instead use the existing dlopen handle on
> liblttng-ust-tracepoint.so to get an handle on a getter and setter
> functions for this value.
> 
> Signed-off-by: Francis Deslauriers 
> ---
> include/lttng/tracepoint.h | 31 ++-
> liblttng-ust/tracepoint.c  | 26 ++
> 2 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
> index 39f2c4d..95f5de9 100644
> --- a/include/lttng/tracepoint.h
> +++ b/include/lttng/tracepoint.h
> @@ -221,26 +221,13 @@ struct lttng_ust_tracepoint_dlopen {
>   void (*rcu_read_lock_sym_bp)(void);
>   void (*rcu_read_unlock_sym_bp)(void);
>   void *(*rcu_dereference_sym_bp)(void *p);
> + void (*tracepoint_set_destructors_disabled)(int is_disabled);

If we expose a "is_disabled" boolean as argument here, it means we want
to allow application to disable and eventually re-enable destructors.
It makes sense, but then we need to keep a reference count within
liblttng-ust-tracepoint.so rather than a simple 1/0 state. Can you
change that ?

This would then be exposed by 3 functions:

Initial state: destructors are enabled, refcount = 1.

void tracepoint_disable_destructors(void)
void tracepoint_enable_destructors(void)
bool tracepoint_get_destructors_state(void)

> + int (*tracepoint_get_destructors_disabled)(void);
> };

Those added fields to:

struct lttng_ust_tracepoint_dlopen tracepoint_dlopen
__attribute__((weak, visibility("hidden")));

mean that if someone has an old .o or .a laying around compiled with the
prior tracepoint_dlopen definition, and links it against a new definition,
there is a mismatch. Arguably, it's limited to within a .so or executable
(module), so typically people recompile, but it won't work if an application
is statically linked against a library: we will have a layout mismatch for
that symbol.

A safer alternative would be to leave this structure unchanged, and have
a new structure that contains those new fields.

> 
> extern struct lttng_ust_tracepoint_dlopen tracepoint_dlopen;
> extern struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr;
> 
> -/* Disable tracepoint destructors. */
> -int __tracepoints__disable_destructors __attribute__((weak));
> -
> -/*
> - * Programs that have threads that survive after they exit, and
> - * therefore call library destructors, should disable the tracepoint
> - * destructors by calling tracepoint_disable_destructors(). This will
> - * leak the tracepoint instrumentation library shared object, leaving
> - * its teardown to the operating system process teardown.
> - */
> -static inline void tracepoint_disable_destructors(void)
> -{
> - __tracepoints__disable_destructors = 1;
> -}
> -
> /*
>  * These weak symbols, the constructor, and destructor take care of
>  * registering only _one_ instance of the tracepoints per shared-ojbect
> @@ -335,7 +322,8 @@ __tracepoints__destroy(void)
>   return;
>   if (!tracepoint_dlopen_ptr)
>   tracepoint_dlopen_ptr = _dlopen;
> - if (!__tracepoints__disable_destructors
> + if (tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled
> + && 
> !tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled()
>   && tracepoint_dlopen_ptr->liblttngust_handle

you can move the check for liblttngust_handle as first check in the if().

>   

[lttng-dev] [PATCH lttng-ust] Fix: destructors do not run on probe provider dlclose

[Francis Deslauriers]

Calling dlclose on a probe provider library does not unregister the
probes from the callsites as the destructors are not executed.

The __tracepoints__disable_destructors weak symbol was exposed by probe
providers, liblttng-ust.so and liblttng-ust-tracepoint.so libraries. If
a probe provider was loaded first into the address space, its definition
would be binded to the symbol. All the subsequent libraries using the
symbol would use the existing definition of the symbol. Thus creating a
a situation where liblttng-ust.so or liblttng-ust-tracepoint.so would
have a dependency on the probe provider library.

This was preventing the dynamic loader from unloading the library as it
was still in use by other libraries. Because of this, the execution of
its destructors and the unregistration of the probes was postponed.

To overcome this issue, we no longer expose this symbol in the
tracepoint.h file to remove the explicit dependency of the probe
provider on the symbol. We instead use the existing dlopen handle on
liblttng-ust-tracepoint.so to get an handle on a getter and setter
functions for this value.

Signed-off-by: Francis Deslauriers 
---
 include/lttng/tracepoint.h | 31 ++-
 liblttng-ust/tracepoint.c  | 26 ++
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
index 39f2c4d..95f5de9 100644
--- a/include/lttng/tracepoint.h
+++ b/include/lttng/tracepoint.h
@@ -221,26 +221,13 @@ struct lttng_ust_tracepoint_dlopen {
void (*rcu_read_lock_sym_bp)(void);
void (*rcu_read_unlock_sym_bp)(void);
void *(*rcu_dereference_sym_bp)(void *p);
+   void (*tracepoint_set_destructors_disabled)(int is_disabled);
+   int (*tracepoint_get_destructors_disabled)(void);
 };
 
 extern struct lttng_ust_tracepoint_dlopen tracepoint_dlopen;
 extern struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr;
 
-/* Disable tracepoint destructors. */
-int __tracepoints__disable_destructors __attribute__((weak));
-
-/*
- * Programs that have threads that survive after they exit, and
- * therefore call library destructors, should disable the tracepoint
- * destructors by calling tracepoint_disable_destructors(). This will
- * leak the tracepoint instrumentation library shared object, leaving
- * its teardown to the operating system process teardown.
- */
-static inline void tracepoint_disable_destructors(void)
-{
-   __tracepoints__disable_destructors = 1;
-}
-
 /*
  * These weak symbols, the constructor, and destructor take care of
  * registering only _one_ instance of the tracepoints per shared-ojbect
@@ -335,7 +322,8 @@ __tracepoints__destroy(void)
return;
if (!tracepoint_dlopen_ptr)
tracepoint_dlopen_ptr = _dlopen;
-   if (!__tracepoints__disable_destructors
+   if (tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled
+   && 
!tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled()
&& tracepoint_dlopen_ptr->liblttngust_handle
&& !__tracepoint_ptrs_registered) {
ret = dlclose(tracepoint_dlopen_ptr->liblttngust_handle);
@@ -423,6 +411,14 @@ __tracepoints__ptrs_init(void)
URCU_FORCE_CAST(int (*)(struct lttng_ust_tracepoint * const *),
dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
"tracepoint_unregister_lib"));
+   tracepoint_dlopen_ptr->tracepoint_set_destructors_disabled =
+   URCU_FORCE_CAST(void (*)(int),
+   dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
+   "tracepoint_set_destructors_disabled"));
+   tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled =
+   URCU_FORCE_CAST(int (*)(void),
+   dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
+   "tracepoint_get_destructors_disabled"));
__tracepoint__init_urcu_sym();
if (tracepoint_dlopen_ptr->tracepoint_register_lib) {

tracepoint_dlopen_ptr->tracepoint_register_lib(__start___tracepoints_ptrs,
@@ -444,7 +440,8 @@ __tracepoints__ptrs_destroy(void)
tracepoint_dlopen_ptr = _dlopen;
if (tracepoint_dlopen_ptr->tracepoint_unregister_lib)

tracepoint_dlopen_ptr->tracepoint_unregister_lib(__start___tracepoints_ptrs);
-   if (!__tracepoints__disable_destructors
+   if (tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled
+   && 
!tracepoint_dlopen_ptr->tracepoint_get_destructors_disabled()
&& tracepoint_dlopen_ptr->liblttngust_handle
&& !__tracepoint_ptrs_registered) {
ret = dlclose(tracepoint_dlopen_ptr->liblttngust_handle);
diff --git