From b43d2e06834a9779801f9a68904ae73a26578f01 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Sat, 21 Sep 2019 10:45:36 +0200 Subject: [PATCH 1/2] gatomic: Add various casts to use of g_atomic_*()s to fix warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When compiling GLib with `-Wsign-conversion`, we get various warnings about the atomic calls. A lot of these were fixed by 3ad375a629c91a27d0165a31f0ed298fd553de0a, but some remain. Fix them by adding appropriate casts at the call sites. Note that `g_atomic_int_{and,or,xor}()` actually all operate on `guint`s rather than `gint`s (which is what the rest of the `g_atomic_int_*()` functions operate on). I can’t find any written reasoning for this, but assume that it’s because signedness is irrelevant when you’re using an integer as a bit field. It’s unfortunate that they’re named a `g_atomic_int_*()` rather than `g_atomic_uint_*()` functions. Tested by compiling GLib as: ``` CFLAGS=-Wsign-conversion jhbuild make -ac |& grep atomic ``` I’m not going to add `-Wsign-conversion` to the set of default warnings for building GLib, because it mostly produces false positives throughout the rest of GLib. Signed-off-by: Philip Withnall Fixes: #1565 --- gio/gdbusconnection.c | 8 ++++---- gio/gdbusnamewatching.c | 4 ++-- gio/tests/gdbus-threading.c | 2 +- glib/gbitlock.c | 2 +- glib/gquark.c | 2 +- glib/gthread-posix.c | 2 +- glib/gthreadpool.c | 10 +++++----- glib/tests/atomic.c | 20 ++++++++++---------- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 117c8df35..1c1f0cf3d 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -3127,7 +3127,7 @@ g_dbus_connection_add_filter (GDBusConnection *connection, CONNECTION_LOCK (connection); data = g_new0 (FilterData, 1); - data->id = g_atomic_int_add (&_global_filter_id, 1); /* TODO: overflow etc. */ + data->id = (guint) g_atomic_int_add (&_global_filter_id, 1); /* TODO: overflow etc. */ data->ref_count = 1; data->filter_function = filter_function; data->user_data = user_data; @@ -3482,7 +3482,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection, subscriber.callback = callback; subscriber.user_data = user_data; subscriber.user_data_free_func = user_data_free_func; - subscriber.id = g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */ + subscriber.id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */ subscriber.context = g_main_context_ref_thread_default (); /* see if we've already have this rule */ @@ -5172,7 +5172,7 @@ g_dbus_connection_register_object (GDBusConnection *connection, } ei = g_new0 (ExportedInterface, 1); - ei->id = g_atomic_int_add (&_global_registration_id, 1); /* TODO: overflow etc. */ + ei->id = (guint) g_atomic_int_add (&_global_registration_id, 1); /* TODO: overflow etc. */ ei->eo = eo; ei->user_data = user_data; ei->user_data_free_func = user_data_free_func; @@ -6832,7 +6832,7 @@ g_dbus_connection_register_subtree (GDBusConnection *connection, es->vtable = _g_dbus_subtree_vtable_copy (vtable); es->flags = flags; - es->id = g_atomic_int_add (&_global_subtree_registration_id, 1); /* TODO: overflow etc. */ + es->id = (guint) g_atomic_int_add (&_global_subtree_registration_id, 1); /* TODO: overflow etc. */ es->user_data = user_data; es->user_data_free_func = user_data_free_func; es->context = g_main_context_ref_thread_default (); diff --git a/gio/gdbusnamewatching.c b/gio/gdbusnamewatching.c index dad8e75ec..01ee6e762 100644 --- a/gio/gdbusnamewatching.c +++ b/gio/gdbusnamewatching.c @@ -603,7 +603,7 @@ g_bus_watch_name (GBusType bus_type, client = g_new0 (Client, 1); client->ref_count = 1; - client->id = g_atomic_int_add (&next_global_id, 1); /* TODO: uh oh, handle overflow */ + client->id = (guint) g_atomic_int_add (&next_global_id, 1); /* TODO: uh oh, handle overflow */ client->name = g_strdup (name); client->flags = flags; client->name_appeared_handler = name_appeared_handler; @@ -665,7 +665,7 @@ guint g_bus_watch_name_on_connection (GDBusConnection *connection, client = g_new0 (Client, 1); client->ref_count = 1; - client->id = g_atomic_int_add (&next_global_id, 1); /* TODO: uh oh, handle overflow */ + client->id = (guint) g_atomic_int_add (&next_global_id, 1); /* TODO: uh oh, handle overflow */ client->name = g_strdup (name); client->flags = flags; client->name_appeared_handler = name_appeared_handler; diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index 3e4dc92e5..13ff15bab 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -514,7 +514,7 @@ test_threaded_singleton (void) /* We want to be the last ref, so let it finish setting up */ for (j = 0; j < 100; j++) { - guint r = g_atomic_int_get (&G_OBJECT (c)->ref_count); + guint r = (guint) g_atomic_int_get (&G_OBJECT (c)->ref_count); if (r == 1) break; diff --git a/glib/gbitlock.c b/glib/gbitlock.c index 46e5f7d06..23024d08c 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -224,7 +224,7 @@ g_bit_lock (volatile gint *address, guint mask = 1u << lock_bit; guint v; - v = g_atomic_int_get (address); + v = (guint) g_atomic_int_get (address); if (v & mask) { guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); diff --git a/glib/gquark.c b/glib/gquark.c index c4d12b870..df12ff69a 100644 --- a/glib/gquark.c +++ b/glib/gquark.c @@ -262,7 +262,7 @@ g_quark_to_string (GQuark quark) gchar **strings; gint seq_id; - seq_id = g_atomic_int_get (&quark_seq_id); + seq_id = (guint) g_atomic_int_get (&quark_seq_id); strings = g_atomic_pointer_get (&quarks); if (quark < seq_id) diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c index 5fff51477..aca9208cc 100644 --- a/glib/gthread-posix.c +++ b/glib/gthread-posix.c @@ -1396,7 +1396,7 @@ void g_cond_wait (GCond *cond, GMutex *mutex) { - guint sampled = g_atomic_int_get (&cond->i[0]); + guint sampled = (guint) g_atomic_int_get (&cond->i[0]); g_mutex_unlock (mutex); syscall (__NR_futex, &cond->i[0], (gsize) FUTEX_WAIT_PRIVATE, (gsize) sampled, NULL); diff --git a/glib/gthreadpool.c b/glib/gthreadpool.c index dd7289370..7d75245b0 100644 --- a/glib/gthreadpool.c +++ b/glib/gthreadpool.c @@ -145,7 +145,7 @@ g_thread_pool_wait_for_new_pool (void) gint last_wakeup_thread_serial; gboolean have_relayed_thread_marker = FALSE; - local_max_unused_threads = g_atomic_int_get (&max_unused_threads); + local_max_unused_threads = (guint) g_atomic_int_get (&max_unused_threads); local_max_idle_time = g_atomic_int_get (&max_idle_time); last_wakeup_thread_serial = g_atomic_int_get (&wakeup_thread_serial); @@ -209,7 +209,7 @@ g_thread_pool_wait_for_new_pool (void) DEBUG_MSG (("thread %p updating to new limits.", g_thread_self ())); - local_max_unused_threads = g_atomic_int_get (&max_unused_threads); + local_max_unused_threads = (guint) g_atomic_int_get (&max_unused_threads); local_max_idle_time = g_atomic_int_get (&max_idle_time); last_wakeup_thread_serial = local_wakeup_thread_serial; @@ -893,7 +893,7 @@ g_thread_pool_get_max_unused_threads (void) guint g_thread_pool_get_num_unused_threads (void) { - return g_atomic_int_get (&unused_threads); + return (guint) g_atomic_int_get (&unused_threads); } /** @@ -1016,7 +1016,7 @@ g_thread_pool_set_max_idle_time (guint interval) g_atomic_int_set (&max_idle_time, interval); - i = g_atomic_int_get (&unused_threads); + i = (guint) g_atomic_int_get (&unused_threads); if (i > 0) { g_atomic_int_inc (&wakeup_thread_serial); @@ -1052,5 +1052,5 @@ g_thread_pool_set_max_idle_time (guint interval) guint g_thread_pool_get_max_idle_time (void) { - return g_atomic_int_get (&max_idle_time); + return (guint) g_atomic_int_get (&max_idle_time); } diff --git a/glib/tests/atomic.c b/glib/tests/atomic.c index 35fa705a4..856df12d2 100644 --- a/glib/tests/atomic.c +++ b/glib/tests/atomic.c @@ -27,7 +27,7 @@ test_types (void) cspp = &csp; g_atomic_int_set (&u, 5); - u2 = g_atomic_int_get (&u); + u2 = (guint) g_atomic_int_get (&u); g_assert_cmpint (u2, ==, 5); res = g_atomic_int_compare_and_exchange (&u, 6, 7); g_assert (!res); @@ -62,13 +62,13 @@ test_types (void) res = g_atomic_int_dec_and_test (&s); g_assert (!res); g_assert_cmpint (s, ==, 6); - s2 = g_atomic_int_and (&s, 5); + s2 = (gint) g_atomic_int_and (&s, 5); g_assert_cmpint (s2, ==, 6); g_assert_cmpint (s, ==, 4); - s2 = g_atomic_int_or (&s, 8); + s2 = (gint) g_atomic_int_or (&s, 8); g_assert_cmpint (s2, ==, 4); g_assert_cmpint (s, ==, 12); - s2 = g_atomic_int_xor (&s, 4); + s2 = (gint) g_atomic_int_xor (&s, 4); g_assert_cmpint (s2, ==, 12); g_assert_cmpint (s, ==, 8); @@ -92,7 +92,7 @@ test_types (void) res = g_atomic_pointer_compare_and_exchange (&gs, 0, 0); g_assert (res); g_assert (gs == 0); - gs2 = g_atomic_pointer_add (&gs, 5); + gs2 = (gsize) g_atomic_pointer_add (&gs, 5); g_assert (gs2 == 0); g_assert (gs == 5); gs2 = g_atomic_pointer_and (&gs, 6); @@ -127,7 +127,7 @@ test_types (void) #undef g_atomic_pointer_xor g_atomic_int_set ((gint*)&u, 5); - u2 = g_atomic_int_get ((gint*)&u); + u2 = (guint) g_atomic_int_get ((gint*)&u); g_assert_cmpint (u2, ==, 5); res = g_atomic_int_compare_and_exchange ((gint*)&u, 6, 7); g_assert (!res); @@ -161,13 +161,13 @@ test_types (void) res = g_atomic_int_dec_and_test (&s); g_assert (!res); g_assert_cmpint (s, ==, 6); - s2 = g_atomic_int_and ((guint*)&s, 5); + s2 = (gint) g_atomic_int_and ((guint*)&s, 5); g_assert_cmpint (s2, ==, 6); g_assert_cmpint (s, ==, 4); - s2 = g_atomic_int_or ((guint*)&s, 8); + s2 = (gint) g_atomic_int_or ((guint*)&s, 8); g_assert_cmpint (s2, ==, 4); g_assert_cmpint (s, ==, 12); - s2 = g_atomic_int_xor ((guint*)&s, 4); + s2 = (gint) g_atomic_int_xor ((guint*)&s, 4); g_assert_cmpint (s2, ==, 12); g_assert_cmpint (s, ==, 8); G_GNUC_BEGIN_IGNORE_DEPRECATIONS @@ -196,7 +196,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS res = g_atomic_pointer_compare_and_exchange (&gs, 0, 0); g_assert (res); g_assert (gs == 0); - gs2 = g_atomic_pointer_add (&gs, 5); + gs2 = (gsize) g_atomic_pointer_add (&gs, 5); g_assert (gs2 == 0); g_assert (gs == 5); gs2 = g_atomic_pointer_and (&gs, 6); -- 2.50.0 From 5d6ced6a7fc6dbbc891992a7d16cd465f2ff1290 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 22 Feb 2023 12:19:16 +0000 Subject: [PATCH 2/2] gdbusconnection: Rearrange refcount handling of map_method_serial_to_task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It already implicitly held a strong ref on its `GTask` values, but didn’t have a free function set so that they would be automatically unreffed on removal from the map. This meant that the functions handling removals from the map, `on_worker_closed()` (via `cancel_method_on_close()`) and `send_message_with_reply_cleanup()` had to call unref once more than they would otherwise. In `send_message_with_reply_cleanup()`, this behaviour depended on whether it was called with `remove == TRUE`. If not, it was `(transfer none)` not `(transfer full)`. This led to bugs in its callers. For example, this led to a direct leak in `cancel_method_on_close()`, as it needed to remove tasks from `map_method_serial_to_task`, but called `send_message_with_reply_cleanup(remove = FALSE)` and erroneously didn’t call unref an additional time. Try and simplify it all by setting a `GDestroyNotify` on `map_method_serial_to_task`’s values, and making the refcount handling of `send_message_with_reply_cleanup()` not be conditional on its arguments. Signed-off-by: Philip Withnall Helps: #1264 --- gio/gdbusconnection.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 1c1f0cf3d..63f37ca4b 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -433,7 +433,7 @@ struct _GDBusConnection GDBusConnectionFlags flags; /* Map used for managing method replies, protected by @lock */ - GHashTable *map_method_serial_to_task; /* guint32 -> GTask* */ + GHashTable *map_method_serial_to_task; /* guint32 -> owned GTask* */ /* Maps used for managing signal subscription, protected by @lock */ GHashTable *map_rule_to_signal_data; /* match rule (gchar*) -> SignalData */ @@ -1067,7 +1067,7 @@ g_dbus_connection_init (GDBusConnection *connection) g_mutex_init (&connection->lock); g_mutex_init (&connection->init_lock); - connection->map_method_serial_to_task = g_hash_table_new (g_direct_hash, g_direct_equal); + connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref); connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash, g_str_equal); @@ -1759,7 +1759,7 @@ send_message_data_free (SendMessageData *data) /* ---------------------------------------------------------------------------------------------------- */ -/* can be called from any thread with lock held; @task is (transfer full) */ +/* can be called from any thread with lock held; @task is (transfer none) */ static void send_message_with_reply_cleanup (GTask *task, gboolean remove) { @@ -1789,13 +1789,11 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove) GUINT_TO_POINTER (data->serial)); g_warn_if_fail (removed); } - - g_object_unref (task); } /* ---------------------------------------------------------------------------------------------------- */ -/* Called from GDBus worker thread with lock held; @task is (transfer full). */ +/* Called from GDBus worker thread with lock held; @task is (transfer none). */ static void send_message_data_deliver_reply_unlocked (GTask *task, GDBusMessage *reply) @@ -1813,7 +1811,7 @@ send_message_data_deliver_reply_unlocked (GTask *task, ; } -/* Called from a user thread, lock is not held */ +/* Called from a user thread, lock is not held; @task is (transfer none) */ static void send_message_data_deliver_error (GTask *task, GQuark domain, @@ -1830,7 +1828,10 @@ send_message_data_deliver_error (GTask *task, return; } + /* Hold a ref on @task as send_message_with_reply_cleanup() will remove it + * from the task map and could end up dropping the last reference */ g_object_ref (task); + send_message_with_reply_cleanup (task, TRUE); CONNECTION_UNLOCK (connection); @@ -2363,7 +2364,8 @@ on_worker_message_about_to_be_sent (GDBusWorker *worker, return message; } -/* called with connection lock held, in GDBusWorker thread */ +/* called with connection lock held, in GDBusWorker thread + * @key, @value and @user_data are (transfer none) */ static gboolean cancel_method_on_close (gpointer key, gpointer value, gpointer user_data) { -- 2.50.0