From 85dbfbbde9dad35fa1bc53f5245ce1e62ddcd6c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 15 Jul 2022 17:49:26 +0200 Subject: [PATCH 1/7] gobject/tests/signals: Add tests for g_object_emitv It's very much used by bindings but we didn't really test it locally. --- gobject/tests/signals.c | 138 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/gobject/tests/signals.c b/gobject/tests/signals.c index 1c7085244a..d9f28b5485 100644 --- a/gobject/tests/signals.c +++ b/gobject/tests/signals.c @@ -1788,6 +1788,143 @@ test_signal_is_valid_name (void) g_assert_false (g_signal_is_valid_name (invalid_names[i])); } +static void +test_emitv (void) +{ + GArray *values; + GObject *test; + GValue return_value = G_VALUE_INIT; + gint count = 0; + guint signal_id; + gulong hook; + gulong id; + + test = g_object_new (test_get_type (), NULL); + + values = g_array_new (TRUE, TRUE, sizeof (GValue)); + g_array_set_clear_func (values, (GDestroyNotify) g_value_unset); + + g_array_set_size (values, 1); + g_value_init (&g_array_index (values, GValue, 0), G_TYPE_OBJECT); + g_value_set_object (&g_array_index (values, GValue, 0), test); + hook = g_signal_add_emission_hook (simple_id, 0, hook_func, &count, NULL); + g_assert_cmpint (count, ==, 0); + g_signal_emitv ((GValue *) values->data, simple_id, 0, NULL); + g_assert_cmpint (count, ==, 1); + g_signal_remove_emission_hook (simple_id, hook); + + g_array_set_size (values, 20); + g_value_init (&g_array_index (values, GValue, 1), G_TYPE_INT); + g_value_set_int (&g_array_index (values, GValue, 1), 42); + + g_value_init (&g_array_index (values, GValue, 2), G_TYPE_BOOLEAN); + g_value_set_boolean (&g_array_index (values, GValue, 2), TRUE); + + g_value_init (&g_array_index (values, GValue, 3), G_TYPE_CHAR); + g_value_set_schar (&g_array_index (values, GValue, 3), 17); + + g_value_init (&g_array_index (values, GValue, 4), G_TYPE_UCHAR); + g_value_set_uchar (&g_array_index (values, GValue, 4), 140); + + g_value_init (&g_array_index (values, GValue, 5), G_TYPE_UINT); + g_value_set_uint (&g_array_index (values, GValue, 5), G_MAXUINT - 42); + + g_value_init (&g_array_index (values, GValue, 6), G_TYPE_LONG); + g_value_set_long (&g_array_index (values, GValue, 6), -1117); + + g_value_init (&g_array_index (values, GValue, 7), G_TYPE_ULONG); + g_value_set_ulong (&g_array_index (values, GValue, 7), G_MAXULONG - 999); + + g_value_init (&g_array_index (values, GValue, 8), enum_type); + g_value_set_enum (&g_array_index (values, GValue, 8), MY_ENUM_VALUE); + + g_value_init (&g_array_index (values, GValue, 9), flags_type); + g_value_set_flags (&g_array_index (values, GValue, 9), + MY_FLAGS_FIRST_BIT | MY_FLAGS_THIRD_BIT | MY_FLAGS_LAST_BIT); + + g_value_init (&g_array_index (values, GValue, 10), G_TYPE_FLOAT); + g_value_set_float (&g_array_index (values, GValue, 10), 0.25); + + g_value_init (&g_array_index (values, GValue, 11), G_TYPE_DOUBLE); + g_value_set_double (&g_array_index (values, GValue, 11), 1.5); + + g_value_init (&g_array_index (values, GValue, 12), G_TYPE_STRING); + g_value_set_string (&g_array_index (values, GValue, 12), "Test"); + + g_value_init (&g_array_index (values, GValue, 13), G_TYPE_PARAM_LONG); + g_value_take_param (&g_array_index (values, GValue, 13), + g_param_spec_long ("param", "nick", "blurb", 0, 10, 4, 0)); + + g_value_init (&g_array_index (values, GValue, 14), G_TYPE_BYTES); + g_value_take_boxed (&g_array_index (values, GValue, 14), + g_bytes_new_static ("Blah", 5)); + + g_value_init (&g_array_index (values, GValue, 15), G_TYPE_POINTER); + g_value_set_pointer (&g_array_index (values, GValue, 15), &enum_type); + + g_value_init (&g_array_index (values, GValue, 16), test_get_type ()); + g_value_set_object (&g_array_index (values, GValue, 16), test); + + g_value_init (&g_array_index (values, GValue, 17), G_TYPE_VARIANT); + g_value_take_variant (&g_array_index (values, GValue, 17), + g_variant_ref_sink (g_variant_new_uint16 (99))); + + g_value_init (&g_array_index (values, GValue, 18), G_TYPE_INT64); + g_value_set_int64 (&g_array_index (values, GValue, 18), G_MAXINT64 - 1234); + + g_value_init (&g_array_index (values, GValue, 19), G_TYPE_UINT64); + g_value_set_uint64 (&g_array_index (values, GValue, 19), G_MAXUINT64 - 123456); + + id = g_signal_connect (test, "all-types", G_CALLBACK (all_types_handler_cb), &flags_type); + signal_id = g_signal_lookup ("all-types", test_get_type ()); + g_assert_cmpuint (signal_id, >, 0); + + count = 0; + hook = g_signal_add_emission_hook (signal_id, 0, hook_func, &count, NULL); + g_assert_cmpint (count, ==, 0); + g_signal_emitv ((GValue *) values->data, signal_id, 0, NULL); + g_assert_cmpint (count, ==, 1); + g_signal_remove_emission_hook (signal_id, hook); + g_clear_signal_handler (&id, test); + + + signal_id = g_signal_lookup ("generic-marshaller-int-return", test_get_type ()); + g_assert_cmpuint (signal_id, >, 0); + g_array_set_size (values, 1); + + id = g_signal_connect (test, + "generic-marshaller-int-return", + G_CALLBACK (on_generic_marshaller_int_return_signed_1), + NULL); + + count = 0; + hook = g_signal_add_emission_hook (signal_id, 0, hook_func, &count, NULL); + g_assert_cmpint (count, ==, 0); + g_value_init (&return_value, G_TYPE_INT); + g_signal_emitv ((GValue *) values->data, signal_id, 0, &return_value); + g_assert_cmpint (count, ==, 1); + g_assert_cmpint (g_value_get_int (&return_value), ==, -30); + g_signal_remove_emission_hook (signal_id, hook); + g_clear_signal_handler (&id, test); + +#ifdef G_ENABLE_DEBUG + g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, + "*return*value*generic-marshaller-int-return*NULL*"); + g_signal_emitv ((GValue *) values->data, signal_id, 0, NULL); + g_test_assert_expected_messages (); + + g_value_unset (&return_value); + g_value_init (&return_value, G_TYPE_FLOAT); + g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, + "*return*value*generic-marshaller-int-return*gfloat*"); + g_signal_emitv ((GValue *) values->data, signal_id, 0, &return_value); + g_test_assert_expected_messages (); +#endif + + g_object_unref (test); + g_array_unref (values); +} + /* --- */ int @@ -1809,6 +1946,7 @@ main (int argc, g_test_add_func ("/gobject/signals/custom-marshaller", test_custom_marshaller); g_test_add_func ("/gobject/signals/connect", test_connect); g_test_add_func ("/gobject/signals/emission-hook", test_emission_hook); + g_test_add_func ("/gobject/signals/emitv", test_emitv); g_test_add_func ("/gobject/signals/accumulator", test_accumulator); g_test_add_func ("/gobject/signals/accumulator-class", test_accumulator_class); g_test_add_func ("/gobject/signals/introspection", test_introspection); -- GitLab From b82a06db92da2984d2bbe9e99bf0247f73df9949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 15 Jul 2022 16:46:38 +0200 Subject: [PATCH 2/7] gsignal: Avoid possible race in g_signal_emit_by_name Since we're locking and unlocking once we've found the signal ID, we might have performed calls to g_signal_emit_valist() with a signal id that was already been removed, and thus failing later. This is not really an issue as inside g_signal_emit_valist() we were re-checking for the signal id, but we can make this more reliable so that the first thread that acquires the lock can also be sure to emit. --- gobject/gsignal.c | 62 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index dd1c6b50d3..30214b1bfd 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -3293,6 +3293,12 @@ accumulate (GSignalInvocationHint *ihint, return continue_emission; } +static gboolean +signal_emit_valist_unlocked (gpointer instance, + guint signal_id, + GQuark detail, + va_list var_args); + /** * g_signal_emit_valist: (skip) * @instance: (type GObject.TypeInstance): the instance the signal is being @@ -3314,6 +3320,26 @@ g_signal_emit_valist (gpointer instance, guint signal_id, GQuark detail, va_list var_args) +{ + SIGNAL_LOCK (); + if (signal_emit_valist_unlocked (instance, signal_id, detail, var_args)) + SIGNAL_UNLOCK (); +} + +/* + * signal_emit_valist_unlocked: + * @instance: The instance to emit from + * @signal_id: Signal id to emit + * @detail: Signal detail + * @var_args: Call arguments + * + * Returns: %TRUE if the signal mutex has been left locked + */ +static gboolean +signal_emit_valist_unlocked (gpointer instance, + guint signal_id, + GQuark detail, + va_list var_args) { GValue *instance_and_params; GType signal_return_type; @@ -3321,23 +3347,20 @@ g_signal_emit_valist (gpointer instance, SignalNode *node; guint i, n_params; - g_return_if_fail (G_TYPE_CHECK_INSTANCE (instance)); - g_return_if_fail (signal_id > 0); + g_return_val_if_fail (G_TYPE_CHECK_INSTANCE (instance), TRUE); + g_return_val_if_fail (signal_id > 0, TRUE); - SIGNAL_LOCK (); node = LOOKUP_SIGNAL_NODE (signal_id); if (!node || !g_type_is_a (G_TYPE_FROM_INSTANCE (instance), node->itype)) { g_critical ("%s: signal id '%u' is invalid for instance '%p'", G_STRLOC, signal_id, instance); - SIGNAL_UNLOCK (); - return; + return TRUE; } #ifndef G_DISABLE_CHECKS if (detail && !(node->flags & G_SIGNAL_DETAILED)) { g_critical ("%s: signal id '%u' does not support detail (%u)", G_STRLOC, signal_id, detail); - SIGNAL_UNLOCK (); - return; + return TRUE; } #endif /* !G_DISABLE_CHECKS */ @@ -3397,10 +3420,7 @@ g_signal_emit_valist (gpointer instance, } if (fastpath && closure == NULL && node->return_type == G_TYPE_NONE) - { - SIGNAL_UNLOCK (); - return; - } + return TRUE; /* Don't allow no-recurse emission as we might have to restart, which means we will run multiple handlers and thus must ref all arguments */ @@ -3476,7 +3496,7 @@ g_signal_emit_valist (gpointer instance, if (fastpath_handler) handler_unref_R (signal_id, instance, fastpath_handler); - SIGNAL_UNLOCK (); + SIGNAL_UNLOCK (); if (accumulator) g_value_unset (&accu); @@ -3514,7 +3534,7 @@ g_signal_emit_valist (gpointer instance, g_object_unref (instance); #endif - return; + return FALSE; } } SIGNAL_UNLOCK (); @@ -3545,7 +3565,7 @@ g_signal_emit_valist (gpointer instance, while (i--) g_value_unset (param_values + i); - return; + return FALSE; } } @@ -3583,6 +3603,8 @@ g_signal_emit_valist (gpointer instance, for (i = 0; i < n_params; i++) g_value_unset (param_values + i); g_value_unset (instance_and_params); + + return FALSE; } /** @@ -3644,19 +3666,23 @@ g_signal_emit_by_name (gpointer instance, SIGNAL_LOCK (); signal_id = signal_parse_name (detailed_signal, itype, &detail, TRUE); - SIGNAL_UNLOCK (); if (signal_id) { va_list var_args; va_start (var_args, detailed_signal); - g_signal_emit_valist (instance, signal_id, detail, var_args); + if (signal_emit_valist_unlocked (instance, signal_id, detail, var_args)) + SIGNAL_UNLOCK (); va_end (var_args); } else - g_critical ("%s: signal name '%s' is invalid for instance '%p' of type '%s'", - G_STRLOC, detailed_signal, instance, g_type_name (itype)); + { + SIGNAL_UNLOCK (); + + g_critical ("%s: signal name '%s' is invalid for instance '%p' of type '%s'", + G_STRLOC, detailed_signal, instance, g_type_name (itype)); + } } static gboolean -- GitLab From e2147df0c92c637e4f33988cb1e91095e158c428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 15 Jul 2022 18:43:02 +0200 Subject: [PATCH 3/7] gsignal: Do not try to access to node members when unlocked In g_signal_emit_valist() we used to access to param types array and n_params values after unlocking the mutex, and this might have lead to making such values unreliable for the current call. So let's keep them around until we're done with the function call --- gobject/gsignal.c | 82 +++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 30214b1bfd..4cd6c001fe 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -3342,10 +3342,9 @@ signal_emit_valist_unlocked (gpointer instance, va_list var_args) { GValue *instance_and_params; - GType signal_return_type; GValue *param_values; SignalNode *node; - guint i, n_params; + guint i; g_return_val_if_fail (G_TYPE_CHECK_INSTANCE (instance), TRUE); g_return_val_if_fail (signal_id > 0, TRUE); @@ -3367,6 +3366,14 @@ signal_emit_valist_unlocked (gpointer instance, if (!node->single_va_closure_is_valid) node_update_single_va_closure (node); + /* There's no need to deep copy this, because a SignalNode instance won't + * ever be destroyed, given that _g_signals_destroy() is not called in any + * real program, however the SignalNode pointer could change, so just store + * the struct contents references, so that we won't try to deference a + * potentially invalid (or changed) pointer; + */ + SignalNode node_copy = *node; + if (node->single_va_closure != NULL) { HandlerList* hlist; @@ -3419,29 +3426,26 @@ signal_emit_valist_unlocked (gpointer instance, } } - if (fastpath && closure == NULL && node->return_type == G_TYPE_NONE) + if (fastpath && closure == NULL && node_copy.return_type == G_TYPE_NONE) return TRUE; /* Don't allow no-recurse emission as we might have to restart, which means we will run multiple handlers and thus must ref all arguments */ - if (closure != NULL && (node->flags & (G_SIGNAL_NO_RECURSE)) != 0) + if (closure != NULL && (node_copy.flags & (G_SIGNAL_NO_RECURSE)) != 0) fastpath = FALSE; if (fastpath) { - SignalAccumulator *accumulator; Emission emission; GValue *return_accu, accu = G_VALUE_INIT; GType instance_type = G_TYPE_FROM_INSTANCE (instance); GValue emission_return = G_VALUE_INIT; - GType rtype = node->return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE; - gboolean static_scope = node->return_type & G_SIGNAL_TYPE_STATIC_SCOPE; + GType rtype = node_copy.return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE; + gboolean static_scope = node_copy.return_type & G_SIGNAL_TYPE_STATIC_SCOPE; - signal_id = node->signal_id; - accumulator = node->accumulator; if (rtype == G_TYPE_NONE) return_accu = NULL; - else if (accumulator) + else if (node_copy.accumulator) return_accu = &accu; else return_accu = &emission_return; @@ -3457,18 +3461,18 @@ signal_emit_valist_unlocked (gpointer instance, if (fastpath_handler) handler_ref (fastpath_handler); - SIGNAL_UNLOCK (); - + if (closure != NULL) + { TRACE(GOBJECT_SIGNAL_EMIT(signal_id, detail, instance, instance_type)); + SIGNAL_UNLOCK (); + if (rtype != G_TYPE_NONE) g_value_init (&emission_return, rtype); - if (accumulator) - g_value_init (&accu, rtype); + if (node_copy.accumulator) + g_value_init (&accu, rtype); - if (closure != NULL) - { /* * Coverity doesn’t understand the paired ref/unref here and seems * to ignore the ref, thus reports every call to g_signal_emit() @@ -3483,12 +3487,15 @@ signal_emit_valist_unlocked (gpointer instance, return_accu, instance, var_args, - node->n_params, - node->param_types); - accumulate (&emission.ihint, &emission_return, &accu, accumulator); - } + node_copy.n_params, + node_copy.param_types); + accumulate (&emission.ihint, &emission_return, &accu, node_copy.accumulator); - SIGNAL_LOCK (); + if (node_copy.accumulator) + g_value_unset (&accu); + + SIGNAL_LOCK (); + } emission.chain_type = G_TYPE_NONE; emission_pop (&emission); @@ -3498,18 +3505,18 @@ signal_emit_valist_unlocked (gpointer instance, SIGNAL_UNLOCK (); - if (accumulator) - g_value_unset (&accu); - if (rtype != G_TYPE_NONE) { gchar *error = NULL; - for (i = 0; i < node->n_params; i++) + for (i = 0; i < node_copy.n_params; i++) { - GType ptype = node->param_types[i] & ~G_SIGNAL_TYPE_STATIC_SCOPE; + GType ptype = node_copy.param_types[i] & ~G_SIGNAL_TYPE_STATIC_SCOPE; G_VALUE_COLLECT_SKIP (ptype, var_args); } + if (closure == NULL) + g_value_init (&emission_return, rtype); + G_VALUE_LCOPY (&emission_return, var_args, static_scope ? G_VALUE_NOCOPY_CONTENTS : 0, @@ -3537,18 +3544,17 @@ signal_emit_valist_unlocked (gpointer instance, return FALSE; } } + SIGNAL_UNLOCK (); - n_params = node->n_params; - signal_return_type = node->return_type; - instance_and_params = g_newa0 (GValue, n_params + 1); + instance_and_params = g_newa0 (GValue, node_copy.n_params + 1); param_values = instance_and_params + 1; - for (i = 0; i < node->n_params; i++) + for (i = 0; i < node_copy.n_params; i++) { gchar *error; - GType ptype = node->param_types[i] & ~G_SIGNAL_TYPE_STATIC_SCOPE; - gboolean static_scope = node->param_types[i] & G_SIGNAL_TYPE_STATIC_SCOPE; + GType ptype = node_copy.param_types[i] & ~G_SIGNAL_TYPE_STATIC_SCOPE; + gboolean static_scope = node_copy.param_types[i] & G_SIGNAL_TYPE_STATIC_SCOPE; G_VALUE_COLLECT_INIT (param_values + i, ptype, var_args, @@ -3571,18 +3577,18 @@ signal_emit_valist_unlocked (gpointer instance, instance_and_params->g_type = 0; g_value_init_from_instance (instance_and_params, instance); - if (signal_return_type == G_TYPE_NONE) - signal_emit_unlocked_R (node, detail, instance, NULL, instance_and_params); + if (node_copy.return_type == G_TYPE_NONE) + signal_emit_unlocked_R (&node_copy, detail, instance, NULL, instance_and_params); else { GValue return_value = G_VALUE_INIT; gchar *error = NULL; - GType rtype = signal_return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE; - gboolean static_scope = signal_return_type & G_SIGNAL_TYPE_STATIC_SCOPE; + GType rtype = node_copy.return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE; + gboolean static_scope = node_copy.return_type & G_SIGNAL_TYPE_STATIC_SCOPE; g_value_init (&return_value, rtype); - signal_emit_unlocked_R (node, detail, instance, &return_value, instance_and_params); + signal_emit_unlocked_R (&node_copy, detail, instance, &return_value, instance_and_params); G_VALUE_LCOPY (&return_value, var_args, @@ -3600,7 +3606,7 @@ signal_emit_valist_unlocked (gpointer instance, */ } } - for (i = 0; i < n_params; i++) + for (i = 0; i < node_copy.n_params; i++) g_value_unset (param_values + i); g_value_unset (instance_and_params); -- GitLab From 8e5f6f32fbc298c317c0c5d17b517f645ba45fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 15 Jul 2022 16:52:40 +0200 Subject: [PATCH 4/7] gsignal: Cleanup g_signal_emitv splitting it in locked and unlocked paths It just makes code easier to maintain and more clearly scoped. --- gobject/gsignal.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 4cd6c001fe..404c98e53e 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -3153,6 +3153,12 @@ g_signal_has_handler_pending (gpointer instance, return has_pending; } +static void +signal_emitv_unlocked (const GValue *instance_and_params, + guint signal_id, + GQuark detail, + GValue *return_value); + /** * g_signal_emitv: * @instance_and_params: (array): argument list for the signal emission. @@ -3175,6 +3181,17 @@ g_signal_emitv (const GValue *instance_and_params, guint signal_id, GQuark detail, GValue *return_value) +{ + SIGNAL_LOCK (); + signal_emitv_unlocked (instance_and_params, signal_id, detail, return_value); + SIGNAL_UNLOCK (); +} + +static void +signal_emitv_unlocked (const GValue *instance_and_params, + guint signal_id, + GQuark detail, + GValue *return_value) { gpointer instance; SignalNode *node; @@ -3192,19 +3209,16 @@ g_signal_emitv (const GValue *instance_and_params, param_values = instance_and_params + 1; #endif - SIGNAL_LOCK (); node = LOOKUP_SIGNAL_NODE (signal_id); if (!node || !g_type_is_a (G_TYPE_FROM_INSTANCE (instance), node->itype)) { g_critical ("%s: signal id '%u' is invalid for instance '%p'", G_STRLOC, signal_id, instance); - SIGNAL_UNLOCK (); return; } #ifdef G_ENABLE_DEBUG if (detail && !(node->flags & G_SIGNAL_DETAILED)) { g_critical ("%s: signal id '%u' does not support detail (%u)", G_STRLOC, signal_id, detail); - SIGNAL_UNLOCK (); return; } for (i = 0; i < node->n_params; i++) @@ -3216,7 +3230,6 @@ g_signal_emitv (const GValue *instance_and_params, i, node->name, G_VALUE_TYPE_NAME (param_values + i)); - SIGNAL_UNLOCK (); return; } if (node->return_type != G_TYPE_NONE) @@ -3227,7 +3240,6 @@ g_signal_emitv (const GValue *instance_and_params, G_STRLOC, type_debug_name (node->return_type), node->name); - SIGNAL_UNLOCK (); return; } else if (!node->accumulator && !G_TYPE_CHECK_VALUE_TYPE (return_value, node->return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE)) @@ -3237,7 +3249,6 @@ g_signal_emitv (const GValue *instance_and_params, type_debug_name (node->return_type), node->name, G_VALUE_TYPE_NAME (return_value)); - SIGNAL_UNLOCK (); return; } } @@ -3264,7 +3275,6 @@ g_signal_emitv (const GValue *instance_and_params, if (hlist == NULL || hlist->handlers == NULL) { /* nothing to do to emit this signal */ - SIGNAL_UNLOCK (); /* g_printerr ("omitting emission of \"%s\"\n", node->name); */ return; } @@ -3272,6 +3282,7 @@ g_signal_emitv (const GValue *instance_and_params, SIGNAL_UNLOCK (); signal_emit_unlocked_R (node, detail, instance, return_value, instance_and_params); + SIGNAL_LOCK (); } static inline gboolean -- GitLab From 832e9c0b0a7e4b24ed9dd17bad060ed1491837ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 15 Jul 2022 21:19:54 +0200 Subject: [PATCH 5/7] gsignal: Reduce lock/unlock operations when calling signal_emit_unlocked_R We used to call this function as unlocked, with a node value that could be invalid at the point of the call, so let's ensure that when we call such function it's defined, and then reduce the access to the signal node members when we're unlocked or after a lock/unlock operation that may have changed it. As per this, add more tests handling multiple signal hooks cases that we did not cover before. --- gobject/gsignal.c | 160 ++++++++++++++++++++++++++++++---------- gobject/tests/signals.c | 90 ++++++++++++++++++++++ 2 files changed, 211 insertions(+), 39 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 404c98e53e..9f50d88587 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -3280,9 +3280,10 @@ signal_emitv_unlocked (const GValue *instance_and_params, } } - SIGNAL_UNLOCK (); - signal_emit_unlocked_R (node, detail, instance, return_value, instance_and_params); - SIGNAL_LOCK (); + /* Pass a stable node pointer, whose address can't change even if the + * g_signal_nodes array gets reallocated. */ + SignalNode node_copy = *node; + signal_emit_unlocked_R (&node_copy, detail, instance, return_value, instance_and_params); } static inline gboolean @@ -3589,7 +3590,11 @@ signal_emit_valist_unlocked (gpointer instance, instance_and_params->g_type = 0; g_value_init_from_instance (instance_and_params, instance); if (node_copy.return_type == G_TYPE_NONE) - signal_emit_unlocked_R (&node_copy, detail, instance, NULL, instance_and_params); + { + SIGNAL_LOCK (); + signal_emit_unlocked_R (&node_copy, detail, instance, NULL, instance_and_params); + SIGNAL_UNLOCK (); + } else { GValue return_value = G_VALUE_INIT; @@ -3599,7 +3604,9 @@ signal_emit_valist_unlocked (gpointer instance, g_value_init (&return_value, rtype); + SIGNAL_LOCK (); signal_emit_unlocked_R (&node_copy, detail, instance, &return_value, instance_and_params); + SIGNAL_UNLOCK (); G_VALUE_LCOPY (&return_value, var_args, @@ -3702,6 +3709,24 @@ g_signal_emit_by_name (gpointer instance, } } +G_ALWAYS_INLINE static inline GValue * +maybe_init_accumulator_unlocked (SignalNode *node, + GValue *emission_return, + GValue *accumulator_value) +{ + if (node->accumulator) + { + if (accumulator_value->g_type) + return accumulator_value; + + g_value_init (accumulator_value, + node->return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE); + return accumulator_value; + } + + return emission_return; +} + static gboolean signal_emit_unlocked_R (SignalNode *node, GQuark detail, @@ -3718,11 +3743,16 @@ signal_emit_unlocked_R (SignalNode *node, guint signal_id; gulong max_sequential_handler_number; gboolean return_value_altered = FALSE; - + guint n_params; + TRACE(GOBJECT_SIGNAL_EMIT(node->signal_id, detail, instance, G_TYPE_FROM_INSTANCE (instance))); - SIGNAL_LOCK (); + /* We expect this function to be called with a stable SignalNode pointer + * that cannot change location, so accessing its stable members should + * always work even after a lock/unlock. + */ signal_id = node->signal_id; + n_params = node->n_params + 1; if (node->flags & G_SIGNAL_NO_RECURSE) { @@ -3731,20 +3761,10 @@ signal_emit_unlocked_R (SignalNode *node, if (emission_node) { emission_node->state = EMISSION_RESTART; - SIGNAL_UNLOCK (); return return_value_altered; } } accumulator = node->accumulator; - if (accumulator) - { - SIGNAL_UNLOCK (); - g_value_init (&accu, node->return_type & ~G_SIGNAL_TYPE_STATIC_SCOPE); - return_accu = &accu; - SIGNAL_LOCK (); - } - else - return_accu = emission_return; emission.instance = instance; emission.ihint.signal_id = node->signal_id; emission.ihint.detail = detail; @@ -3772,9 +3792,10 @@ signal_emit_unlocked_R (SignalNode *node, emission.chain_type = G_TYPE_FROM_INSTANCE (instance); SIGNAL_UNLOCK (); + return_accu = maybe_init_accumulator_unlocked (node, emission_return, &accu); g_closure_invoke (class_closure, return_accu, - node->n_params + 1, + n_params, instance_and_params, &emission.ihint); if (!accumulate (&emission.ihint, emission_return, &accu, accumulator) && @@ -3789,35 +3810,94 @@ signal_emit_unlocked_R (SignalNode *node, else if (emission.state == EMISSION_RESTART) goto EMIT_RESTART; } - + if (node->emission_hooks) { - gboolean need_destroy, was_in_call, may_recurse = TRUE; GHook *hook; + GHook **emission_hooks = NULL; + guint8 *hook_returns = NULL; + size_t n_emission_hooks = 0; + const gboolean may_recurse = TRUE; + guint i; emission.state = EMISSION_HOOK; + + /* Quick check to determine whether any hooks match this emission, + * before committing to the more complex work of calling those hooks. + */ hook = g_hook_first_valid (node->emission_hooks, may_recurse); while (hook) { SignalHook *signal_hook = SIGNAL_HOOK (hook); - + if (!signal_hook->detail || signal_hook->detail == detail) - { - GSignalEmissionHook hook_func = (GSignalEmissionHook) hook->func; - - was_in_call = G_HOOK_IN_CALL (hook); - hook->flags |= G_HOOK_FLAG_IN_CALL; - SIGNAL_UNLOCK (); - need_destroy = !hook_func (&emission.ihint, node->n_params + 1, instance_and_params, hook->data); - SIGNAL_LOCK (); - if (!was_in_call) - hook->flags &= ~G_HOOK_FLAG_IN_CALL; - if (need_destroy) - g_hook_destroy_link (node->emission_hooks, hook); - } + n_emission_hooks += 1; + hook = g_hook_next_valid (node->emission_hooks, hook, may_recurse); } - + + if G_UNLIKELY (n_emission_hooks > 0) + { + emission_hooks = g_newa (GHook *, n_emission_hooks); + hook_returns = g_newa (guint8, n_emission_hooks); + + /* Re-iterate back through the matching hooks and copy them into + * an array which won’t change when we unlock to call the + * user-provided hook functions. + * These functions may change hook configuration for this signal, + * add / remove signal handlers, etc. + */ + hook = g_hook_first_valid (node->emission_hooks, may_recurse); + for (i = 0; hook; ) + { + SignalHook *signal_hook = SIGNAL_HOOK (hook); + + if (!signal_hook->detail || signal_hook->detail == detail) + emission_hooks[i++] = g_hook_ref (node->emission_hooks, hook); + + hook = g_hook_next_valid (node->emission_hooks, hook, may_recurse); + } + + g_assert (i == n_emission_hooks); + + SIGNAL_UNLOCK (); + + for (i = 0; i < n_emission_hooks; ++i) + { + GSignalEmissionHook hook_func; + gboolean need_destroy; + guint old_flags; + + hook = emission_hooks[i]; + hook_func = (GSignalEmissionHook) hook->func; + + old_flags = g_atomic_int_or (&hook->flags, G_HOOK_FLAG_IN_CALL); + need_destroy = !hook_func (&emission.ihint, n_params, + instance_and_params, hook->data); + + if (!(old_flags & G_HOOK_FLAG_IN_CALL)) + { + g_atomic_int_compare_and_exchange (&hook->flags, + old_flags | G_HOOK_FLAG_IN_CALL, + old_flags); + } + + hook_returns[i] = !!need_destroy; + } + + SIGNAL_LOCK (); + + for (i = 0; i < n_emission_hooks; i++) + { + hook = emission_hooks[i]; + + g_hook_unref (node->emission_hooks, hook); + + if (hook_returns[i]) + g_hook_destroy_link (node->emission_hooks, hook); + } + } + if (emission.state == EMISSION_RESTART) goto EMIT_RESTART; } @@ -3842,9 +3922,10 @@ signal_emit_unlocked_R (SignalNode *node, handler->sequential_number < max_sequential_handler_number) { SIGNAL_UNLOCK (); + return_accu = maybe_init_accumulator_unlocked (node, emission_return, &accu); g_closure_invoke (handler->closure, return_accu, - node->n_params + 1, + n_params, instance_and_params, &emission.ihint); if (!accumulate (&emission.ihint, emission_return, &accu, accumulator) && @@ -3881,9 +3962,10 @@ signal_emit_unlocked_R (SignalNode *node, emission.chain_type = G_TYPE_FROM_INSTANCE (instance); SIGNAL_UNLOCK (); + return_accu = maybe_init_accumulator_unlocked (node, emission_return, &accu); g_closure_invoke (class_closure, return_accu, - node->n_params + 1, + n_params, instance_and_params, &emission.ihint); if (!accumulate (&emission.ihint, emission_return, &accu, accumulator) && @@ -3913,9 +3995,10 @@ signal_emit_unlocked_R (SignalNode *node, handler->sequential_number < max_sequential_handler_number) { SIGNAL_UNLOCK (); + return_accu = maybe_init_accumulator_unlocked (node, emission_return, &accu); g_closure_invoke (handler->closure, return_accu, - node->n_params + 1, + n_params, instance_and_params, &emission.ihint); if (!accumulate (&emission.ihint, emission_return, &accu, accumulator) && @@ -3962,7 +4045,7 @@ signal_emit_unlocked_R (SignalNode *node, } g_closure_invoke (class_closure, node->return_type != G_TYPE_NONE ? &accu : NULL, - node->n_params + 1, + n_params, instance_and_params, &emission.ihint); if (!accumulate (&emission.ihint, emission_return, &accu, accumulator) && @@ -3983,7 +4066,6 @@ signal_emit_unlocked_R (SignalNode *node, handler_unref_R (signal_id, instance, handler_list); emission_pop (&emission); - SIGNAL_UNLOCK (); if (accumulator) g_value_unset (&accu); diff --git a/gobject/tests/signals.c b/gobject/tests/signals.c index d9f28b5485..c511da3e9b 100644 --- a/gobject/tests/signals.c +++ b/gobject/tests/signals.c @@ -1130,12 +1130,35 @@ hook_func (GSignalInvocationHint *ihint, return TRUE; } +static gboolean +hook_func_removal (GSignalInvocationHint *ihint, + guint n_params, + const GValue *params, + gpointer data) +{ + gint *count = data; + + (*count)++; + + return FALSE; +} + +static void +simple_handler_remove_hook (GObject *sender, + gpointer data) +{ + gulong *hook = data; + + g_signal_remove_emission_hook (simple_id, *hook); +} + static void test_emission_hook (void) { GObject *test1, *test2; gint count = 0; gulong hook; + gulong connection_id; test1 = g_object_new (test_get_type (), NULL); test2 = g_object_new (test_get_type (), NULL); @@ -1150,6 +1173,73 @@ test_emission_hook (void) g_signal_emit_by_name (test1, "simple"); g_assert_cmpint (count, ==, 2); + count = 0; + hook = g_signal_add_emission_hook (simple_id, 0, hook_func_removal, &count, NULL); + g_assert_cmpint (count, ==, 0); + g_signal_emit_by_name (test1, "simple"); + g_assert_cmpint (count, ==, 1); + g_signal_emit_by_name (test2, "simple"); + g_assert_cmpint (count, ==, 1); + + g_test_expect_message ("GLib-GObject", G_LOG_LEVEL_CRITICAL, + "*simple* had no hook * to remove"); + g_signal_remove_emission_hook (simple_id, hook); + g_test_assert_expected_messages (); + + count = 0; + hook = g_signal_add_emission_hook (simple_id, 0, hook_func, &count, NULL); + connection_id = g_signal_connect (test1, "simple", + G_CALLBACK (simple_handler_remove_hook), &hook); + g_assert_cmpint (count, ==, 0); + g_signal_emit_by_name (test1, "simple"); + g_assert_cmpint (count, ==, 1); + g_signal_emit_by_name (test2, "simple"); + g_assert_cmpint (count, ==, 1); + + g_test_expect_message ("GLib-GObject", G_LOG_LEVEL_CRITICAL, + "*simple* had no hook * to remove"); + g_signal_remove_emission_hook (simple_id, hook); + g_test_assert_expected_messages (); + + g_clear_signal_handler (&connection_id, test1); + + gulong hooks[10]; + count = 0; + + for (size_t i = 0; i < G_N_ELEMENTS (hooks); ++i) + hooks[i] = g_signal_add_emission_hook (simple_id, 0, hook_func, &count, NULL); + + g_assert_cmpint (count, ==, 0); + g_signal_emit_by_name (test1, "simple"); + g_assert_cmpint (count, ==, 10); + g_signal_emit_by_name (test2, "simple"); + g_assert_cmpint (count, ==, 20); + + for (size_t i = 0; i < G_N_ELEMENTS (hooks); ++i) + g_signal_remove_emission_hook (simple_id, hooks[i]); + + g_signal_emit_by_name (test1, "simple"); + g_assert_cmpint (count, ==, 20); + + count = 0; + + for (size_t i = 0; i < G_N_ELEMENTS (hooks); ++i) + hooks[i] = g_signal_add_emission_hook (simple_id, 0, hook_func_removal, &count, NULL); + + g_assert_cmpint (count, ==, 0); + g_signal_emit_by_name (test1, "simple"); + g_assert_cmpint (count, ==, 10); + g_signal_emit_by_name (test2, "simple"); + g_assert_cmpint (count, ==, 10); + + for (size_t i = 0; i < G_N_ELEMENTS (hooks); ++i) + { + g_test_expect_message ("GLib-GObject", G_LOG_LEVEL_CRITICAL, + "*simple* had no hook * to remove"); + g_signal_remove_emission_hook (simple_id, hooks[i]); + g_test_assert_expected_messages (); + } + g_object_unref (test1); g_object_unref (test2); } -- GitLab From 3788431a620d358007b95315a84b6b3988ea46fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 21 Dec 2022 03:52:59 +0100 Subject: [PATCH 6/7] gsignal: Use static allocation if we've just few emission hooks Normally we don't really have emission hooks around, so try to allocate only tiny array to contain a few of them and in case we exceed that limit, we go back to use allocated ones. --- gobject/gsignal.c | 77 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 9f50d88587..63e70d0b83 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -3814,8 +3814,7 @@ signal_emit_unlocked_R (SignalNode *node, if (node->emission_hooks) { GHook *hook; - GHook **emission_hooks = NULL; - guint8 *hook_returns = NULL; + GHook *static_emission_hooks[3]; size_t n_emission_hooks = 0; const gboolean may_recurse = TRUE; guint i; @@ -3824,6 +3823,8 @@ signal_emit_unlocked_R (SignalNode *node, /* Quick check to determine whether any hooks match this emission, * before committing to the more complex work of calling those hooks. + * We save a few of them into a static array, to try to avoid further + * allocations. */ hook = g_hook_first_valid (node->emission_hooks, may_recurse); while (hook) @@ -3831,35 +3832,71 @@ signal_emit_unlocked_R (SignalNode *node, SignalHook *signal_hook = SIGNAL_HOOK (hook); if (!signal_hook->detail || signal_hook->detail == detail) - n_emission_hooks += 1; + { + if (n_emission_hooks < G_N_ELEMENTS (static_emission_hooks)) + { + static_emission_hooks[n_emission_hooks] = + g_hook_ref (node->emission_hooks, hook); + } + + n_emission_hooks += 1; + } hook = g_hook_next_valid (node->emission_hooks, hook, may_recurse); } + /* Re-iterate back through the matching hooks and copy them into + * an array which won’t change when we unlock to call the + * user-provided hook functions. + * These functions may change hook configuration for this signal, + * add / remove signal handlers, etc. + */ if G_UNLIKELY (n_emission_hooks > 0) { - emission_hooks = g_newa (GHook *, n_emission_hooks); - hook_returns = g_newa (guint8, n_emission_hooks); - - /* Re-iterate back through the matching hooks and copy them into - * an array which won’t change when we unlock to call the - * user-provided hook functions. - * These functions may change hook configuration for this signal, - * add / remove signal handlers, etc. - */ - hook = g_hook_first_valid (node->emission_hooks, may_recurse); - for (i = 0; hook; ) + guint8 static_hook_returns[G_N_ELEMENTS (static_emission_hooks)]; + GHook **emission_hooks = NULL; + guint8 *hook_returns = NULL; + + if G_LIKELY (n_emission_hooks <= G_N_ELEMENTS (static_emission_hooks)) { - SignalHook *signal_hook = SIGNAL_HOOK (hook); + emission_hooks = static_emission_hooks; + hook_returns = static_hook_returns; + } + else + { + emission_hooks = g_newa (GHook *, n_emission_hooks); + hook_returns = g_newa (guint8, n_emission_hooks); - if (!signal_hook->detail || signal_hook->detail == detail) - emission_hooks[i++] = g_hook_ref (node->emission_hooks, hook); + /* We can't just memcpy the ones we have in the static array, + * to the alloca()'d one because otherwise we'd get an invalid + * ID assertion during unref + */ + hook = g_hook_first_valid (node->emission_hooks, may_recurse); + for (i = 0; hook; ) + { + SignalHook *signal_hook = SIGNAL_HOOK (hook); + + if (!signal_hook->detail || signal_hook->detail == detail) + { + if (i < G_N_ELEMENTS (static_emission_hooks)) + { + emission_hooks[i] = g_steal_pointer (&static_emission_hooks[i]); + g_assert (emission_hooks[i] == hook); + } + else + { + emission_hooks[i] = g_hook_ref (node->emission_hooks, hook); + } + + i += 1; + } + + hook = g_hook_next_valid (node->emission_hooks, hook, may_recurse); + } - hook = g_hook_next_valid (node->emission_hooks, hook, may_recurse); + g_assert (i == n_emission_hooks); } - g_assert (i == n_emission_hooks); - SIGNAL_UNLOCK (); for (i = 0; i < n_emission_hooks; ++i) -- GitLab From 16c5a11bf6c9ff2e6dc4f967714b296654c2dc8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 21 Dec 2022 04:27:56 +0100 Subject: [PATCH 7/7] gsignals: Do not zero two times the instance GValue's GType We already use g_new0 to create such arrays and nothing writes in the first pointer till this point, so no need to zero it again. --- gobject/gsignal.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 63e70d0b83..7387739ef8 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -2305,7 +2305,6 @@ g_signal_chain_from_overridden_handler (gpointer instance, } SIGNAL_UNLOCK (); - instance_and_params->g_type = 0; g_value_init_from_instance (instance_and_params, instance); SIGNAL_LOCK (); @@ -3587,7 +3586,6 @@ signal_emit_valist_unlocked (gpointer instance, } } - instance_and_params->g_type = 0; g_value_init_from_instance (instance_and_params, instance); if (node_copy.return_type == G_TYPE_NONE) { -- GitLab