diff mbox series

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

Message ID 1519251048-23222-1-git-send-email-francis.deslauriers@efficios.com
State Superseded, archived
Headers show
Series [lttng-ust] Fix: destructors do not run on probe provider dlclose | expand

Commit Message

Francis Deslauriers Feb. 21, 2018, 10:10 p.m. UTC
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 <francis.deslauriers at efficios.com>
---
 include/lttng/tracepoint.h | 31 ++++++++++++++-----------------
 liblttng-ust/tracepoint.c  | 26 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 17 deletions(-)

Comments

Mathieu Desnoyers Feb. 22, 2018, 1:34 p.m. UTC | #1
----- On Feb 21, 2018, at 5:10 PM, Francis Deslauriers francis.deslauriers at 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 <francis.deslauriers at efficios.com>
> ---
> 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 = &tracepoint_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().

> 			&& !__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 = &tracepoint_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

same here.

> 			&& !__tracepoint_ptrs_registered) {
> 		ret = dlclose(tracepoint_dlopen_ptr->liblttngust_handle);
> diff --git a/liblttng-ust/tracepoint.c b/liblttng-ust/tracepoint.c
> index 2c25d46..5d9e10e 100644
> --- a/liblttng-ust/tracepoint.c
> +++ b/liblttng-ust/tracepoint.c
> @@ -53,6 +53,22 @@ struct {
> /* Set to 1 to enable tracepoint debug output */
> static const int tracepoint_debug;
> static int initialized;
> +
> +/*
> + * Programs that have threads that survive after they exit, and
> + * therefore call library destructors, should disable the tracepoint
> + * destructors by calling tracepoints_set_destructors_disabled(). This will
> + * leak the tracepoint instrumentation library shared object, leaving its
> + * teardown to the operating system process teardown.
> + *
> + * Set to 1 to disable the execution of tracepoint library destructors.
> + *
> + * To access and/or modify this value, users need to use a combination of
> + * dlopen(3) and dlsym(3) to get an handle on the
> + * tracepoint_set_destructors_disabled and tracepoint_get_destructors_disabled
> + * symbols below.
> + */
> +static int tracepoint_destructors_disabled;

This would become:

static int tracepoint_destructors_refcount = 1;

And accessed through uatomic_*() APIs. I would use uatomic
rather than liburcu refcount API because we don't want to run
anything when the refcount reaches 0.

Thanks,

Mathieu

> static void (*new_tracepoint_cb)(struct lttng_ust_tracepoint *);
> 
> /*
> @@ -166,6 +182,16 @@ static void debug_print_probes(struct tracepoint_entry
> *entry)
> 		DBG("Probe %d : %p", i, entry->probes[i].func);
> }
> 
> +void tracepoint_set_destructors_disabled(int is_disabled)
> +{
> +	tracepoint_destructors_disabled = is_disabled;
> +}
> +
> +int tracepoint_get_destructors_disabled(void)
> +{
> +	return tracepoint_destructors_disabled;
> +}
> +
> static void *
> tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> 			   void (*probe)(void), void *data)
> --
> 2.7.4
diff mbox series

Patch

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 = &tracepoint_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 = &tracepoint_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 a/liblttng-ust/tracepoint.c b/liblttng-ust/tracepoint.c
index 2c25d46..5d9e10e 100644
--- a/liblttng-ust/tracepoint.c
+++ b/liblttng-ust/tracepoint.c
@@ -53,6 +53,22 @@  struct {
 /* Set to 1 to enable tracepoint debug output */
 static const int tracepoint_debug;
 static int initialized;
+
+/*
+ * Programs that have threads that survive after they exit, and
+ * therefore call library destructors, should disable the tracepoint
+ * destructors by calling tracepoints_set_destructors_disabled(). This will
+ * leak the tracepoint instrumentation library shared object, leaving its
+ * teardown to the operating system process teardown.
+ *
+ * Set to 1 to disable the execution of tracepoint library destructors.
+ *
+ * To access and/or modify this value, users need to use a combination of
+ * dlopen(3) and dlsym(3) to get an handle on the
+ * tracepoint_set_destructors_disabled and tracepoint_get_destructors_disabled
+ * symbols below.
+ */
+static int tracepoint_destructors_disabled;
 static void (*new_tracepoint_cb)(struct lttng_ust_tracepoint *);
 
 /*
@@ -166,6 +182,16 @@  static void debug_print_probes(struct tracepoint_entry *entry)
 		DBG("Probe %d : %p", i, entry->probes[i].func);
 }
 
+void tracepoint_set_destructors_disabled(int is_disabled)
+{
+	tracepoint_destructors_disabled = is_disabled;
+}
+
+int tracepoint_get_destructors_disabled(void)
+{
+	return tracepoint_destructors_disabled;
+}
+
 static void *
 tracepoint_entry_add_probe(struct tracepoint_entry *entry,
 			   void (*probe)(void), void *data)